[clang] 2903a8a - [CGTypes] Remove recursion protection
Nikita Popov via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 16 04:07:24 PDT 2023
Author: Nikita Popov
Date: 2023-06-16T13:07:14+02:00
New Revision: 2903a8ab07260f9836db82b7e5330f1892830aca
URL: https://github.com/llvm/llvm-project/commit/2903a8ab07260f9836db82b7e5330f1892830aca
DIFF: https://github.com/llvm/llvm-project/commit/2903a8ab07260f9836db82b7e5330f1892830aca.diff
LOG: [CGTypes] Remove recursion protection
With opaque pointers, it should no longer be necessary to protect
against recursion when converting Clang types to LLVM types, as
recursion can only be introduced via pointer types.
Differential Revision: https://reviews.llvm.org/D152999
Added:
Modified:
clang/lib/CodeGen/CodeGenTypes.cpp
clang/lib/CodeGen/CodeGenTypes.h
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/CodeGenTypes.cpp b/clang/lib/CodeGen/CodeGenTypes.cpp
index e4836c850a157..30021794a0bb3 100644
--- a/clang/lib/CodeGen/CodeGenTypes.cpp
+++ b/clang/lib/CodeGen/CodeGenTypes.cpp
@@ -125,93 +125,9 @@ bool CodeGenTypes::isRecordLayoutComplete(const Type *Ty) const {
return I != RecordDeclTypes.end() && !I->second->isOpaque();
}
-static bool
-isSafeToConvert(QualType T, CodeGenTypes &CGT,
- llvm::SmallPtrSet<const RecordDecl*, 16> &AlreadyChecked);
-
-
-/// isSafeToConvert - Return true if it is safe to convert the specified record
-/// decl to IR and lay it out, false if doing so would cause us to get into a
-/// recursive compilation mess.
-static bool
-isSafeToConvert(const RecordDecl *RD, CodeGenTypes &CGT,
- llvm::SmallPtrSet<const RecordDecl*, 16> &AlreadyChecked) {
- // If we have already checked this type (maybe the same type is used by-value
- // multiple times in multiple structure fields, don't check again.
- if (!AlreadyChecked.insert(RD).second)
- return true;
-
- const Type *Key = CGT.getContext().getTagDeclType(RD).getTypePtr();
-
- // If this type is already laid out, converting it is a noop.
- if (CGT.isRecordLayoutComplete(Key)) return true;
-
- // If this type is currently being laid out, we can't recursively compile it.
- if (CGT.isRecordBeingLaidOut(Key))
- return false;
-
- // If this type would require laying out bases that are currently being laid
- // out, don't do it. This includes virtual base classes which get laid out
- // when a class is translated, even though they aren't embedded by-value into
- // the class.
- if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
- for (const auto &I : CRD->bases())
- if (!isSafeToConvert(I.getType()->castAs<RecordType>()->getDecl(), CGT,
- AlreadyChecked))
- return false;
- }
-
- // If this type would require laying out members that are currently being laid
- // out, don't do it.
- for (const auto *I : RD->fields())
- if (!isSafeToConvert(I->getType(), CGT, AlreadyChecked))
- return false;
-
- // If there are no problems, lets do it.
- return true;
-}
-
-/// isSafeToConvert - Return true if it is safe to convert this field type,
-/// which requires the structure elements contained by-value to all be
-/// recursively safe to convert.
-static bool
-isSafeToConvert(QualType T, CodeGenTypes &CGT,
- llvm::SmallPtrSet<const RecordDecl*, 16> &AlreadyChecked) {
- // Strip off atomic type sugar.
- if (const auto *AT = T->getAs<AtomicType>())
- T = AT->getValueType();
-
- // If this is a record, check it.
- if (const auto *RT = T->getAs<RecordType>())
- return isSafeToConvert(RT->getDecl(), CGT, AlreadyChecked);
-
- // If this is an array, check the elements, which are embedded inline.
- if (const auto *AT = CGT.getContext().getAsArrayType(T))
- return isSafeToConvert(AT->getElementType(), CGT, AlreadyChecked);
-
- // Otherwise, there is no concern about transforming this. We only care about
- // things that are contained by-value in a structure that can have another
- // structure as a member.
- return true;
-}
-
-
-/// isSafeToConvert - Return true if it is safe to convert the specified record
-/// decl to IR and lay it out, false if doing so would cause us to get into a
-/// recursive compilation mess.
-static bool isSafeToConvert(const RecordDecl *RD, CodeGenTypes &CGT) {
- // If no structs are being laid out, we can certainly do this one.
- if (CGT.noRecordsBeingLaidOut()) return true;
-
- llvm::SmallPtrSet<const RecordDecl*, 16> AlreadyChecked;
- return isSafeToConvert(RD, CGT, AlreadyChecked);
-}
-
/// isFuncParamTypeConvertible - Return true if the specified type in a
/// function parameter or result position can be converted to an IR type at this
-/// point. This boils down to being whether it is complete, as well as whether
-/// we've temporarily deferred expanding the type because we're in a recursive
-/// context.
+/// point. This boils down to being whether it is complete.
bool CodeGenTypes::isFuncParamTypeConvertible(QualType Ty) {
// Some ABIs cannot have their member pointers represented in IR unless
// certain circumstances have been reached.
@@ -223,21 +139,7 @@ bool CodeGenTypes::isFuncParamTypeConvertible(QualType Ty) {
if (!TT) return true;
// Incomplete types cannot be converted.
- if (TT->isIncompleteType())
- return false;
-
- // If this is an enum, then it is always safe to convert.
- const RecordType *RT = dyn_cast<RecordType>(TT);
- if (!RT) return true;
-
- // Otherwise, we have to be careful. If it is a struct that we're in the
- // process of expanding, then we can't convert the function type. That's ok
- // though because we must be in a pointer context under the struct, so we can
- // just convert it to a dummy type.
- //
- // We decide this by checking whether ConvertRecordDeclType returns us an
- // opaque type for a struct that we know is defined.
- return isSafeToConvert(RT->getDecl(), *this);
+ return !TT->isIncompleteType();
}
@@ -333,7 +235,6 @@ static llvm::Type *getTypeForFormat(llvm::LLVMContext &VMContext,
llvm::Type *CodeGenTypes::ConvertFunctionTypeInternal(QualType QFT) {
assert(QFT.isCanonical());
- const Type *Ty = QFT.getTypePtr();
const FunctionType *FT = cast<FunctionType>(QFT.getTypePtr());
// First, check whether we can build the full function type. If the
// function type depends on an incomplete type (e.g. a struct or enum), we
@@ -356,14 +257,6 @@ llvm::Type *CodeGenTypes::ConvertFunctionTypeInternal(QualType QFT) {
return llvm::StructType::get(getLLVMContext());
}
- // While we're converting the parameter types for a function, we don't want
- // to recursively convert any pointed-to structs. Converting directly-used
- // structs is ok though.
- if (!RecordsBeingLaidOut.insert(Ty).second) {
- SkippedLayout = true;
- return llvm::StructType::get(getLLVMContext());
- }
-
// The function type can be built; call the appropriate routines to
// build it.
const CGFunctionInfo *FI;
@@ -389,11 +282,6 @@ llvm::Type *CodeGenTypes::ConvertFunctionTypeInternal(QualType QFT) {
ResultType = GetFunctionType(*FI);
}
- RecordsBeingLaidOut.erase(Ty);
-
- if (RecordsBeingLaidOut.empty())
- while (!DeferredRecords.empty())
- ConvertRecordDeclType(DeferredRecords.pop_back_val());
return ResultType;
}
@@ -421,27 +309,16 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
if (const RecordType *RT = dyn_cast<RecordType>(Ty))
return ConvertRecordDeclType(RT->getDecl());
- // 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;
- // With expensive checks, check that the type we compute matches the
- // cached type.
+ auto TCI = TypeCache.find(Ty);
+ if (TCI != TypeCache.end())
+ CachedType = TCI->second;
+ // With expensive checks, check that the type we compute matches the
+ // cached type.
#ifndef EXPENSIVE_CHECKS
- if (CachedType)
- return CachedType;
+ if (CachedType)
+ return CachedType;
#endif
- }
// If we don't have it in the cache, convert it now.
llvm::Type *ResultType = nullptr;
@@ -835,8 +712,7 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
assert((!CachedType || CachedType == ResultType) &&
"Cached type doesn't match computed type");
- if (ShouldUseCache)
- TypeCache[Ty] = ResultType;
+ TypeCache[Ty] = ResultType;
return ResultType;
}
@@ -869,17 +745,6 @@ llvm::StructType *CodeGenTypes::ConvertRecordDeclType(const RecordDecl *RD) {
if (!RD || !RD->isCompleteDefinition() || !Ty->isOpaque())
return Ty;
- // If converting this type would cause us to infinitely loop, don't do it!
- if (!isSafeToConvert(RD, *this)) {
- DeferredRecords.push_back(RD);
- return Ty;
- }
-
- // Okay, this is a definition of a type. Compile the implementation now.
- bool InsertResult = RecordsBeingLaidOut.insert(Key).second;
- (void)InsertResult;
- assert(InsertResult && "Recursively compiling a struct?");
-
// Force conversion of non-virtual base classes recursively.
if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
for (const auto &I : CRD->bases()) {
@@ -892,22 +757,12 @@ llvm::StructType *CodeGenTypes::ConvertRecordDeclType(const RecordDecl *RD) {
std::unique_ptr<CGRecordLayout> Layout = ComputeRecordLayout(RD, Ty);
CGRecordLayouts[Key] = std::move(Layout);
- // We're done laying out this struct.
- bool EraseResult = RecordsBeingLaidOut.erase(Key); (void)EraseResult;
- assert(EraseResult && "struct not in RecordsBeingLaidOut set?");
-
// If this struct blocked a FunctionType conversion, then recompute whatever
// was derived from that.
// FIXME: This is hugely overconservative.
if (SkippedLayout)
TypeCache.clear();
- // If we're done converting the outer-most record, then convert any deferred
- // structs as well.
- if (RecordsBeingLaidOut.empty())
- while (!DeferredRecords.empty())
- ConvertRecordDeclType(DeferredRecords.pop_back_val());
-
return Ty;
}
diff --git a/clang/lib/CodeGen/CodeGenTypes.h b/clang/lib/CodeGen/CodeGenTypes.h
index e76fda95513f6..9088f77b95c34 100644
--- a/clang/lib/CodeGen/CodeGenTypes.h
+++ b/clang/lib/CodeGen/CodeGenTypes.h
@@ -78,20 +78,12 @@ class CodeGenTypes {
/// Hold memoized CGFunctionInfo results.
llvm::FoldingSet<CGFunctionInfo> FunctionInfos{FunctionInfosLog2InitSize};
- /// This set keeps track of records that we're currently converting
- /// to an IR type. For example, when converting:
- /// struct A { struct B { int x; } } when processing 'x', the 'A' and 'B'
- /// types will be in this set.
- llvm::SmallPtrSet<const Type*, 4> RecordsBeingLaidOut;
-
llvm::SmallPtrSet<const CGFunctionInfo*, 4> FunctionsBeingProcessed;
/// True if we didn't layout a function due to a being inside
/// a recursive struct conversion, set this to true.
bool SkippedLayout;
- SmallVector<const RecordDecl *, 8> DeferredRecords;
-
/// This map keeps cache of llvm::Types and maps clang::Type to
/// corresponding llvm::Type.
llvm::DenseMap<const Type *, llvm::Type *> TypeCache;
@@ -300,12 +292,6 @@ class CodeGenTypes {
bool isZeroInitializable(const RecordDecl *RD);
bool isRecordLayoutComplete(const Type *Ty) const;
- bool noRecordsBeingLaidOut() const {
- return RecordsBeingLaidOut.empty();
- }
- bool isRecordBeingLaidOut(const Type *Ty) const {
- return RecordsBeingLaidOut.count(Ty);
- }
unsigned getTargetAddressSpace(QualType T) const;
};
More information about the cfe-commits
mailing list