[PATCH] D118744: [clang] Don't cache function type after clearing clang->llvm type cache

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We clear the type cache when SkippedLayout is true and we're converting
a function type. However, we then immediately put the computed entry in
the cache. A later query with different context may incorrectly use the
cached entry. So don't cache the entry in that case.

For example in the test case we give up converting the exact function
type when seeing z(*)() in the context of z::p, but not g().

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.

Fixes PR53465.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118744

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenCXX/pr53465.cpp


Index: clang/test/CodeGenCXX/pr53465.cpp
===================================================================
--- /dev/null
+++ clang/test/CodeGenCXX/pr53465.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -mllvm -verify-type-cache -emit-llvm %s -o - -triple i386-pc-windows-msvc19.16.0 | FileCheck %s
+// REQUIRES: asserts
+
+// CHECK: call {}* @"?f@@YA?AUz@@XZ"()
+
+struct z {
+  z (*p)();
+};
+
+z f();
+
+void g() {
+  f();
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -25,9 +25,20 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+
 using namespace clang;
 using namespace CodeGen;
 
+#ifndef NDEBUG
+// TODO: turn on by default when defined(EXPENSIVE_CHECKS) once check-clang is
+// 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,12 +393,10 @@
 
   RecordsBeingLaidOut.erase(Ty);
 
-  if (SkippedLayout)
-    TypeCache.clear();
-
   if (RecordsBeingLaidOut.empty())
     while (!DeferredRecords.empty())
       ConvertRecordDeclType(DeferredRecords.pop_back_val());
+
   return ResultType;
 }
 
@@ -418,8 +427,16 @@
   // 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;
+
+  llvm::Type *CachedType = TCI != TypeCache.end() ? TCI->second : nullptr;
+  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;
@@ -797,7 +814,18 @@
 
   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 (SkippedLayout && (Ty->getTypeClass() == Type::FunctionNoProto ||
+                        Ty->getTypeClass() == Type::FunctionProto))
+    TypeCache.clear();
+  else
+    TypeCache[Ty] = ResultType;
   return ResultType;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D118744.405125.patch
Type: text/x-patch
Size: 2642 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220202/ce3c4490/attachment.bin>