llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Fangrui Song (MaskRay) <details> <summary>Changes</summary> Some clang code calls FoldingSetImpl::insert with a node whose profile doesn't match its insert token, leading to missing hash-consing. * getObjCTypeParamType keys on Decl->getUnderlyingType() where Profile() reports getCanonicalTypeInternal(). Canonicalize before the lookup: the decl's underlying type can be updated later. * getUnaryTransformType drops UnderlyingType for a dependent base after building the ID. * getFunctionTypeInternal keys canonically where Profile() passes isCanonicalUnqualified(), which differs for a computed noexcept. * getConstantArrayType keys on the full index qualifiers; only the three CVR bits are stored. * LoanManager keys a PlaceholderBase on the bare decl pointer where Profile() reports the PointerUnion's opaque value. These issues would be caught by #<!-- -->220166 Aided by Opus 5 --- Full diff: https://github.com/llvm/llvm-project/pull/220168.diff 4 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h (+5-1) - (modified) clang/lib/AST/ASTContext.cpp (+20-11) - (modified) clang/lib/AST/Type.cpp (+3-1) - (modified) clang/lib/Analysis/LifetimeSafety/Loans.cpp (+2-2) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h index 8137a207290d7..784968789d40a 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h @@ -89,9 +89,13 @@ class PlaceholderBase : public llvm::FoldingSetNode { return ParamOrMethod.dyn_cast<const CXXMethodDecl *>(); } - void Profile(llvm::FoldingSetNodeID &ID) const { + using KeyTy = llvm::PointerUnion<const ParmVarDecl *, const CXXMethodDecl *>; + + static void Profile(llvm::FoldingSetNodeID &ID, KeyTy ParamOrMethod) { ID.AddPointer(ParamOrMethod.getOpaqueValue()); } + + void Profile(llvm::FoldingSetNodeID &ID) const { Profile(ID, ParamOrMethod); } }; /// Represents the storage location being borrowed, e.g., a specific stack diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index b7e771595e86e..819e2527a26d5 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -4287,6 +4287,10 @@ QualType ASTContext::getConstantArrayType(QualType EltTy, llvm::APInt ArySize(ArySizeIn); ArySize = ArySize.zextOrTrunc(Target->getMaxPointerWidth()); + // The type stores only the CVR bits of the index qualifiers, so key on + // those. + IndexTypeQuals &= Qualifiers::CVRMask; + llvm::FoldingSetNodeID ID; ConstantArrayType::Profile(ID, *this, EltTy, ArySize.getZExtValue(), SizeExpr, ASM, IndexTypeQuals); @@ -5701,8 +5705,8 @@ UnresolvedUsingType *ASTContext::getUnresolvedUsingTypeInternal( auto *T = new (Mem) UnresolvedUsingType(Keyword, Qualifier, D, CanonicalType); if (Token) { auto *Placeholder = new (T->getFoldingSetPlaceholder()) - FoldingSetPlaceholder<TypedefType>(); - TypedefTypes.insert(Placeholder, Token); + FoldingSetPlaceholder<UnresolvedUsingType>(); + UnresolvedUsingTypes.insert(Placeholder, Token); } Types.push_back(T); return T; @@ -6534,13 +6538,6 @@ ASTContext::applyObjCProtocolQualifiers(QualType type, QualType ASTContext::getObjCTypeParamType(const ObjCTypeParamDecl *Decl, ArrayRef<ObjCProtocolDecl *> protocols) const { - // Look in the folding set for an existing type. - llvm::FoldingSetNodeID ID; - ObjCTypeParamType::Profile(ID, Decl, Decl->getUnderlyingType(), protocols); - llvm::FoldingSetInsertToken Token; - if (ObjCTypeParamType *TypeParam = ObjCTypeParamTypes.lookup(ID, Token)) - return QualType(TypeParam, 0); - // We canonicalize to the underlying type. QualType Canonical = getCanonicalType(Decl->getUnderlyingType()); if (!protocols.empty()) { @@ -6551,6 +6548,14 @@ ASTContext::getObjCTypeParamType(const ObjCTypeParamDecl *Decl, assert(!hasError && "Error when apply protocol qualifier to bound type"); } + // Key on the canonical type the node is constructed with, which is what + // Profile() reports; the decl's underlying type can be updated later. + llvm::FoldingSetNodeID ID; + ObjCTypeParamType::Profile(ID, Decl, Canonical, protocols); + llvm::FoldingSetInsertToken Token; + if (ObjCTypeParamType *TypeParam = ObjCTypeParamTypes.lookup(ID, Token)) + return QualType(TypeParam, 0); + unsigned size = sizeof(ObjCTypeParamType); size += protocols.size() * sizeof(ObjCProtocolDecl *); void *mem = Allocate(size, alignof(ObjCTypeParamType)); @@ -6837,6 +6842,12 @@ QualType ASTContext::getPackIndexingType(QualType Pattern, Expr *IndexExpr, QualType ASTContext::getUnaryTransformType(QualType BaseType, QualType UnderlyingType, UnaryTransformType::UTTKind Kind) const { + // Clear UnderlyingType for a dependent base before building the ID: that is + // what the node is constructed with, and what Profile() reports. + if (BaseType->isDependentType()) { + assert(UnderlyingType.isNull() || BaseType == UnderlyingType); + UnderlyingType = QualType(); + } llvm::FoldingSetNodeID ID; UnaryTransformType::Profile(ID, BaseType, UnderlyingType, Kind); @@ -6849,8 +6860,6 @@ ASTContext::getUnaryTransformType(QualType BaseType, QualType UnderlyingType, if (!BaseType->isDependentType()) { CanonType = UnderlyingType.getCanonicalType(); } else { - assert(UnderlyingType.isNull() || BaseType == UnderlyingType); - UnderlyingType = QualType(); if (QualType CanonBase = BaseType.getCanonicalType(); BaseType != CanonBase) { CanonType = getUnaryTransformType(CanonBase, QualType(), Kind); diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index df296eb4e28e5..f1f8d79fba957 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/lib/AST/Type.cpp @@ -4115,8 +4115,10 @@ void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, QualType Result, void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx) { + // getFunctionTypeInternal keys every node canonically and compares noexcept + // expressions after the lookup, so report that same key. Profile(ID, getReturnType(), param_type_begin(), getNumParams(), - getExtProtoInfo(), Ctx, isCanonicalUnqualified()); + getExtProtoInfo(), Ctx, /*Canonical=*/true); } TypeCoupledDeclRefInfo::TypeCoupledDeclRefInfo(ValueDecl *D, bool Deref) diff --git a/clang/lib/Analysis/LifetimeSafety/Loans.cpp b/clang/lib/Analysis/LifetimeSafety/Loans.cpp index 3a850ca8a9b74..e6cbd922d20c7 100644 --- a/clang/lib/Analysis/LifetimeSafety/Loans.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Loans.cpp @@ -38,7 +38,7 @@ void Loan::dump(llvm::raw_ostream &OS) const { const PlaceholderBase * LoanManager::getOrCreatePlaceholderBase(const ParmVarDecl *PVD) { llvm::FoldingSetNodeID ID; - ID.AddPointer(PVD); + PlaceholderBase::Profile(ID, PVD); llvm::FoldingSetInsertToken InsertToken; if (PlaceholderBase *Existing = PlaceholderBases.lookup(ID, InsertToken)) return Existing; @@ -52,7 +52,7 @@ LoanManager::getOrCreatePlaceholderBase(const ParmVarDecl *PVD) { const PlaceholderBase * LoanManager::getOrCreatePlaceholderBase(const CXXMethodDecl *MD) { llvm::FoldingSetNodeID ID; - ID.AddPointer(MD); + PlaceholderBase::Profile(ID, MD); llvm::FoldingSetInsertToken InsertToken; if (PlaceholderBase *Existing = PlaceholderBases.lookup(ID, InsertToken)) return Existing; `````````` </details> https://github.com/llvm/llvm-project/pull/220168 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
