This revision was automatically updated to reflect the committed changes.
Closed by commit rG2903a8ab0726: [CGTypes] Remove recursion protection
(authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152999/new/
https://reviews.llvm.org/D152999
Files:
clang/lib/CodeGen/CodeGenTypes.cpp
clang/lib/CodeGen/CodeGenTypes.h
Index: clang/lib/CodeGen/CodeGenTypes.h
===================================================================
--- clang/lib/CodeGen/CodeGenTypes.h
+++ clang/lib/CodeGen/CodeGenTypes.h
@@ -78,20 +78,12 @@
/// 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 @@
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;
};
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -125,93 +125,9 @@
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 @@
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 @@
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 @@
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 @@
ResultType = GetFunctionType(*FI);
}
- RecordsBeingLaidOut.erase(Ty);
-
- if (RecordsBeingLaidOut.empty())
- while (!DeferredRecords.empty())
- ConvertRecordDeclType(DeferredRecords.pop_back_val());
return ResultType;
}
@@ -421,27 +309,16 @@
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 @@
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 @@
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 @@
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
