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;
}
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
