llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) <details> <summary>Changes</summary> Whenever we add a new call to e.g. `ASTContext::getASTRecordLayout()`, we inevitably cause a problem because that function has quite a few prerequisites: ```c++ D = D->getDefinition(); assert(D && "Cannot get layout of forward declarations!"); assert(!D->isInvalidDecl() && "Cannot get layout of invalid decl!"); assert(D->isCompleteDefinition() && "Cannot layout type before complete!"); ``` Add a function to check whether a record decl can be pased to `getASTRecordLayout()` and update a few callers. --- Full diff: https://github.com/llvm/llvm-project/pull/216541.diff 7 Files Affected: - (modified) clang/include/clang/AST/ASTContext.h (+7) - (modified) clang/lib/AST/ByteCode/Pointer.cpp (+8-2) - (modified) clang/lib/AST/ExprConstant.cpp (+4) - (modified) clang/lib/AST/RecordLayoutBuilder.cpp (-3) - (modified) clang/lib/Sema/SemaChecking.cpp (+1-1) - (modified) clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp (+3) - (modified) clang/lib/StaticAnalyzer/Core/MemRegion.cpp (+2-2) ``````````diff diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 56b51566f58f5..5ecfdf0567055 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -2930,9 +2930,16 @@ class ASTContext : public RefCountedBase<ASTContext> { /// [[gnu::ms_struct]]. bool defaultsToMsStruct() const; + /// Whether layout (offset and size) information can be queried for \p D. + static bool hasLayout(const RecordDecl *D) { + D = D->getDefinition(); + return D && !D->isInvalidDecl() && D->isCompleteDefinition(); + } + /// Get or compute information about the layout of the specified /// record (struct/union/class) \p D, which indicates its size and field /// position information. + /// \pre hasLayout(D) const ASTRecordLayout &getASTRecordLayout(const RecordDecl *D) const; /// Get or compute information about the layout of the specified diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp index 4f36d20b352cb..6df64da62e0e1 100644 --- a/clang/lib/AST/ByteCode/Pointer.cpp +++ b/clang/lib/AST/ByteCode/Pointer.cpp @@ -423,6 +423,8 @@ Pointer::computeOffsetForComparison(const ASTContext &ASTCtx) const { const Record *R = P.getBase().getRecord(); assert(R); + if (!ASTContext::hasLayout(R->getDecl())) + return std::nullopt; const ASTRecordLayout &Layout = ASTCtx.getASTRecordLayout(R->getDecl()); Result += ASTCtx .toCharUnitsFromBits( @@ -481,8 +483,10 @@ Pointer::computeLayoutOffset(const ASTContext &ASTCtx) const { PtrView P = view(); while (true) { if (P.isBaseClass()) { - const ASTRecordLayout &Layout = - ASTCtx.getASTRecordLayout(getRecordDecl(P.getBase())); + const CXXRecordDecl *BaseRD = getRecordDecl(P.getBase()); + if (!ASTContext::hasLayout(BaseRD)) + return std::nullopt; + const ASTRecordLayout &Layout = ASTCtx.getASTRecordLayout(BaseRD); const CXXRecordDecl *RD = getRecordDecl(P); if (P.isVirtualBaseClass()) Result += Layout.getVBaseClassOffset(RD).getQuantity(); @@ -522,6 +526,8 @@ Pointer::computeLayoutOffset(const ASTContext &ASTCtx) const { assert(P.getField()); const FieldDecl *F = P.getField(); + if (!ASTContext::hasLayout(F->getParent())) + return std::nullopt; const ASTRecordLayout &Layout = ASTCtx.getASTRecordLayout(F->getParent()); Result += ASTCtx.toCharUnitsFromBits(Layout.getFieldOffset(F->getFieldIndex())) diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 480d5119a5363..a727b42f52890 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/clang/lib/AST/ExprConstant.cpp @@ -7613,6 +7613,8 @@ static bool HandleDestructionImpl(EvalInfo &Info, SourceRange CallRange, if (RD->isUnion()) return true; + if (!ASTContext::hasLayout(RD)) + return false; const ASTRecordLayout &Layout = Info.Ctx.getASTRecordLayout(RD); // We don't have a good way to iterate fields in reverse, so collect all the @@ -8007,6 +8009,8 @@ class APValueToBufferConverter { bool visitRecord(const APValue &Val, QualType Ty, CharUnits Offset) { const RecordDecl *RD = Ty->getAsRecordDecl(); + if (!ASTContext::hasLayout(RD)) + return false; const ASTRecordLayout &Layout = Info.Ctx.getASTRecordLayout(RD); // Visit the base classes. diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp index c27572bc7f50d..e6da6c78238c1 100644 --- a/clang/lib/AST/RecordLayoutBuilder.cpp +++ b/clang/lib/AST/RecordLayoutBuilder.cpp @@ -3423,9 +3423,6 @@ bool ASTContext::defaultsToMsStruct() const { getTargetInfo().getTriple().isWindowsGNUEnvironment(); } -/// getASTRecordLayout - Get or compute information about the layout of the -/// specified record (struct/union/class), which indicates its size and field -/// position information. const ASTRecordLayout & ASTContext::getASTRecordLayout(const RecordDecl *D) const { if (D->hasExternalLexicalStorage() && !D->getDefinition()) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 3e6266b8ac542..d44f4b054856a 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -15744,7 +15744,7 @@ std::optional<std::pair< auto *ME = cast<MemberExpr>(E); auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl()); if (!FD || FD->getType()->isReferenceType() || - FD->getParent()->isInvalidDecl()) + !ASTContext::hasLayout(FD->getParent())) break; std::optional<std::pair<CharUnits, CharUnits>> P; if (ME->isArrow()) diff --git a/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp index 1554604b374ca..c8d7b6555f3f4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -76,6 +76,9 @@ class PaddingChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { if (!(RD = RD->getDefinition())) return; + if (RD->isInvalidDecl()) + return; + // This is the simplest correct case: a class with no fields and one base // class. Other cases are more complicated because of how the base classes // & fields might interact, so we don't bother dealing with them. diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp index 9f27358381738..36a71d510b902 100644 --- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp @@ -1638,7 +1638,7 @@ static RegionOffset calculateOffset(const MemRegion *R) { } const CXXRecordDecl *Child = Ty->getAsCXXRecordDecl(); - if (!Child) { + if (!Child || !ASTContext::hasLayout(Child)) { // We cannot compute the offset of the base class. SymbolicOffsetBase = R; } else { @@ -1712,7 +1712,7 @@ static RegionOffset calculateOffset(const MemRegion *R) { assert(R); const RecordDecl *RD = FR->getDecl()->getParent(); - if (RD->isUnion() || !RD->isCompleteDefinition()) { + if (RD->isUnion() || !ASTContext::hasLayout(RD)) { // We cannot compute offset for incomplete type. // For unions, we could treat everything as offset 0, but we'd rather // treat each field as a symbolic offset so they aren't stored on top `````````` </details> https://github.com/llvm/llvm-project/pull/216541 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
