Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 31 May 2022 15:55:49 GMT
From:      Dimitry Andric <dim@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 9ac7261611ad - stable/12 - Apply clang fix for assertion failure building putty 0.77 on i386
Message-ID:  <202205311555.24VFtn4q015558@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/12 has been updated by dim:

URL: https://cgit.FreeBSD.org/src/commit/?id=9ac7261611ad130fcfae0da803e2e85c1b9c73f1

commit 9ac7261611ad130fcfae0da803e2e85c1b9c73f1
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2022-05-28 21:26:37 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2022-05-31 13:17:17 +0000

    Apply clang fix for assertion failure building putty 0.77 on i386
    
    Merge commit 45084eab5e63 from llvm git (by Arthur Eubanks):
    
      [clang] Fix some clang->llvm type cache invalidation issues
    
      Take the following as an example
    
        struct z {
          z (*p)();
        };
    
        z f();
    
      When we attempt to get the LLVM type of f, we recurse into z. z itself
      has a function pointer with the same type as f. Given the recursion,
      Clang simply treats z::p as a pointer to an empty struct `{}*`. The
      LLVM type of f is as expected. So we have two different potential
      LLVM types for a given Clang type. If we store one of those into the
      cache, when we access the cache with a different context (e.g. we
      are/aren't recursing on z) we may get an incorrect result. There is some
      attempt to clear the cache in these cases, but it doesn't seem to handle
      all cases.
    
      This change makes it so we only use the cache when we are not in any
      sort of function context, i.e. `noRecordsBeingLaidOut() &&
      FunctionsBeingProcessed.empty()`, which are the cases where we may
      decide to choose a different LLVM type for a given Clang type. LLVM
      types for builtin types are never recursive so they're always ok.
    
      This allows us to clear the type cache less often (as seen with the
      removal of one of the calls to `TypeCache.clear()`). We
      still need to clear it when we use a placeholder type then replace it
      later with the final type and other dependent types need to be
      recalculated.
    
      I've added a check that the cached type matches what we compute. It
      triggered in this test case without the fix. It's currently not
      check-clang clean so it's not on by default for something like expensive
      checks builds.
    
      This change uncovered another issue where the LLVM types for an argument
      and its local temporary don't match. For example in type-cache-3, when
      expanding z::dc's argument into a temporary alloca, we ConvertType() the
      type of z::p which is `void ({}*)*`, which doesn't match the alloca GEP
      type of `{}*`.
    
      No noticeable compile time changes:
      https://llvm-compile-time-tracker.com/compare.php?from=3918dd6b8acf8c5886b9921138312d1c638b2937&to=50bdec9836ed40e38ece0657f3058e730adffc4c&stat=instructions
    
      Fixes #53465.
    
      Reviewed By: rnk
    
      Differential Revision: https://reviews.llvm.org/D118744
    
    PR:             264318
    Reported by:    mandree
    MFC after:      3 days
    
    (cherry picked from commit 6a5eebc190ab98de98ed7977cbdee3218758376e)
---
 contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h |  5 ++-
 contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp  | 18 ++++++--
 .../clang/lib/CodeGen/CodeGenTypes.cpp             | 52 ++++++++++++++++++----
 3 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h b/contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h
index 4fad44a105cd..9331c03b1838 100644
--- a/contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h
+++ b/contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h
@@ -9,10 +9,11 @@
 #ifndef LLVM_CLANG_LIB_CODEGEN_CGBUILDER_H
 #define LLVM_CLANG_LIB_CODEGEN_CGBUILDER_H
 
-#include "llvm/IR/DataLayout.h"
-#include "llvm/IR/IRBuilder.h"
 #include "Address.h"
 #include "CodeGenTypeCache.h"
+#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/Type.h"
 
 namespace clang {
 namespace CodeGen {
diff --git a/contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp b/contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp
index 47a4ed35be85..638b4e0897ec 100644
--- a/contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp
+++ b/contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp
@@ -38,6 +38,7 @@
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/Type.h"
 #include "llvm/Transforms/Utils/Local.h"
 using namespace clang;
 using namespace CodeGen;
@@ -1056,10 +1057,19 @@ void CodeGenFunction::ExpandTypeFromArgs(QualType Ty, LValue LV,
     // Call EmitStoreOfScalar except when the lvalue is a bitfield to emit a
     // primitive store.
     assert(isa<NoExpansion>(Exp.get()));
-    if (LV.isBitField())
-      EmitStoreThroughLValue(RValue::get(&*AI++), LV);
-    else
-      EmitStoreOfScalar(&*AI++, LV);
+    llvm::Value *Arg = &*AI++;
+    if (LV.isBitField()) {
+      EmitStoreThroughLValue(RValue::get(Arg), LV);
+    } else {
+      // TODO: currently there are some places are inconsistent in what LLVM
+      // pointer type they use (see D118744). Once clang uses opaque pointers
+      // all LLVM pointer types will be the same and we can remove this check.
+      if (Arg->getType()->isPointerTy()) {
+        Address Addr = LV.getAddress(*this);
+        Arg = Builder.CreateBitCast(Arg, Addr.getElementType());
+      }
+      EmitStoreOfScalar(Arg, LV);
+    }
   }
 }
 
diff --git a/contrib/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp b/contrib/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp
index 9cb42941cb96..5b1551ea0355 100644
--- a/contrib/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp
+++ b/contrib/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp
@@ -25,9 +25,20 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Module.h"
+
 using namespace clang;
 using namespace CodeGen;
 
+#ifndef NDEBUG
+#include "llvm/Support/CommandLine.h"
+// TODO: turn on by default when defined(EXPENSIVE_CHECKS) once check-clang is
+// -verify-type-cache clean.
+static llvm::cl::opt<bool> VerifyTypeCache(
+    "verify-type-cache",
+    llvm::cl::desc("Verify that the type cache matches the computed type"),
+    llvm::cl::init(false), llvm::cl::Hidden);
+#endif
+
 CodeGenTypes::CodeGenTypes(CodeGenModule &cgm)
   : CGM(cgm), Context(cgm.getContext()), TheModule(cgm.getModule()),
     Target(cgm.getTarget()), TheCXXABI(cgm.getCXXABI()),
@@ -382,9 +393,6 @@ llvm::Type *CodeGenTypes::ConvertFunctionTypeInternal(QualType QFT) {
 
   RecordsBeingLaidOut.erase(Ty);
 
-  if (SkippedLayout)
-    TypeCache.clear();
-
   if (RecordsBeingLaidOut.empty())
     while (!DeferredRecords.empty())
       ConvertRecordDeclType(DeferredRecords.pop_back_val());
@@ -415,11 +423,29 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
   if (const RecordType *RT = dyn_cast<RecordType>(Ty))
     return ConvertRecordDeclType(RT->getDecl());
 
-  // See if type is already cached.
-  llvm::DenseMap<const Type *, llvm::Type *>::iterator TCI = TypeCache.find(Ty);
-  // If type is found in map then use it. Otherwise, convert type T.
-  if (TCI != TypeCache.end())
-    return TCI->second;
+  // The LLVM type we return for a given Clang type may not always be the same,
+  // most notably when dealing with recursive structs. We mark these potential
+  // cases with ShouldUseCache below. Builtin types cannot be recursive.
+  // TODO: when clang uses LLVM opaque pointers we won't be able to represent
+  // recursive types with LLVM types, making this logic much simpler.
+  llvm::Type *CachedType = nullptr;
+  bool ShouldUseCache =
+      Ty->isBuiltinType() ||
+      (noRecordsBeingLaidOut() && FunctionsBeingProcessed.empty());
+  if (ShouldUseCache) {
+    llvm::DenseMap<const Type *, llvm::Type *>::iterator TCI =
+        TypeCache.find(Ty);
+    if (TCI != TypeCache.end())
+      CachedType = TCI->second;
+    if (CachedType) {
+#ifndef NDEBUG
+      if (!VerifyTypeCache)
+        return CachedType;
+#else
+      return CachedType;
+#endif
+    }
+  }
 
   // If we don't have it in the cache, convert it now.
   llvm::Type *ResultType = nullptr;
@@ -794,7 +820,15 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
 
   assert(ResultType && "Didn't convert a type?");
 
-  TypeCache[Ty] = ResultType;
+#ifndef NDEBUG
+  if (CachedType) {
+    assert(CachedType == ResultType &&
+           "Cached type doesn't match computed type");
+  }
+#endif
+
+  if (ShouldUseCache)
+    TypeCache[Ty] = ResultType;
   return ResultType;
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202205311555.24VFtn4q015558>