Could there be an issue with Objective C property attributes? https://crbug.com/1134762
On Sun, Oct 11, 2020 at 2:11 PM Richard Smith <rich...@metafoo.co.uk> wrote: > Please can you try building with https://reviews.llvm.org/D89212 applied > first, and see if it produces any warnings? If so, you're probably hitting > PR47663, and should fix that by moving the 'weak' attribute earlier. > > On Sun, 11 Oct 2020 at 11:18, Richard Smith <rich...@metafoo.co.uk> wrote: > >> On Fri, 9 Oct 2020 at 19:12, Arthur Eubanks <aeuba...@google.com> wrote: >> >>> I think this is the cause of https://crbug.com/1134762. Can we >>> speculatively revert while coming up with a repro? Or would you like a >>> repro first? >>> >> >> If you're blocked on this, sure, please go ahead and revert until you >> have a repro. But the revert will block ongoing development work, so a >> reproducer would definitely be appreciated! Per PR47663, there are some >> pretty fundamental problems with adding the 'weak' attribute "too late", so >> if that's what's going on, the best approach might be to move the 'weak' >> attribute to an earlier declaration. >> >> On Sun, Sep 27, 2020 at 7:06 PM Richard Smith via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> >>>> >>>> Author: Richard Smith >>>> Date: 2020-09-27T19:05:26-07:00 >>>> New Revision: 9dcd96f728863d40d6f5922ed52732fdd728fb5f >>>> >>>> URL: >>>> https://github.com/llvm/llvm-project/commit/9dcd96f728863d40d6f5922ed52732fdd728fb5f >>>> DIFF: >>>> https://github.com/llvm/llvm-project/commit/9dcd96f728863d40d6f5922ed52732fdd728fb5f.diff >>>> >>>> LOG: Canonicalize declaration pointers when forming APValues. >>>> >>>> References to different declarations of the same entity aren't different >>>> values, so shouldn't have different representations. >>>> >>>> Recommit of e6393ee813178e9d3306b8e3c6949a4f32f8a2cb with fixed handling >>>> for weak declarations. We now look for attributes on the most recent >>>> declaration when determining whether a declaration is weak. (Second >>>> recommit with further fixes for mishandling of weak declarations. Our >>>> behavior here is fundamentally unsound -- see PR47663 -- but this >>>> approach attempts to not make things worse.) >>>> >>>> Added: >>>> >>>> >>>> Modified: >>>> clang/include/clang/AST/APValue.h >>>> clang/lib/AST/APValue.cpp >>>> clang/lib/AST/Decl.cpp >>>> clang/lib/AST/DeclBase.cpp >>>> clang/lib/AST/ExprConstant.cpp >>>> clang/lib/CodeGen/CGExprConstant.cpp >>>> clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp >>>> clang/test/CodeGenCXX/weak-external.cpp >>>> clang/test/OpenMP/ordered_messages.cpp >>>> >>>> Removed: >>>> >>>> >>>> >>>> >>>> ################################################################################ >>>> diff --git a/clang/include/clang/AST/APValue.h >>>> b/clang/include/clang/AST/APValue.h >>>> index 5103cfa8604e..6307f8a92e5a 100644 >>>> --- a/clang/include/clang/AST/APValue.h >>>> +++ b/clang/include/clang/AST/APValue.h >>>> @@ -174,6 +174,7 @@ class APValue { >>>> return !(LHS == RHS); >>>> } >>>> friend llvm::hash_code hash_value(const LValueBase &Base); >>>> + friend struct llvm::DenseMapInfo<LValueBase>; >>>> >>>> private: >>>> PtrTy Ptr; >>>> @@ -201,8 +202,7 @@ class APValue { >>>> >>>> public: >>>> LValuePathEntry() : Value() {} >>>> - LValuePathEntry(BaseOrMemberType BaseOrMember) >>>> - : >>>> Value{reinterpret_cast<uintptr_t>(BaseOrMember.getOpaqueValue())} {} >>>> + LValuePathEntry(BaseOrMemberType BaseOrMember); >>>> static LValuePathEntry ArrayIndex(uint64_t Index) { >>>> LValuePathEntry Result; >>>> Result.Value = Index; >>>> >>>> diff --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp >>>> index 08ae0ff3c67d..32d3ff7ce1d0 100644 >>>> --- a/clang/lib/AST/APValue.cpp >>>> +++ b/clang/lib/AST/APValue.cpp >>>> @@ -38,7 +38,7 @@ static_assert( >>>> "Type is insufficiently aligned"); >>>> >>>> APValue::LValueBase::LValueBase(const ValueDecl *P, unsigned I, >>>> unsigned V) >>>> - : Ptr(P), Local{I, V} {} >>>> + : Ptr(P ? cast<ValueDecl>(P->getCanonicalDecl()) : nullptr), >>>> Local{I, V} {} >>>> APValue::LValueBase::LValueBase(const Expr *P, unsigned I, unsigned V) >>>> : Ptr(P), Local{I, V} {} >>>> >>>> @@ -82,13 +82,19 @@ bool operator==(const APValue::LValueBase &LHS, >>>> const APValue::LValueBase &RHS) { >>>> if (LHS.Ptr != RHS.Ptr) >>>> return false; >>>> - if (LHS.is<TypeInfoLValue>()) >>>> + if (LHS.is<TypeInfoLValue>() || LHS.is<DynamicAllocLValue>()) >>>> return true; >>>> return LHS.Local.CallIndex == RHS.Local.CallIndex && >>>> LHS.Local.Version == RHS.Local.Version; >>>> } >>>> } >>>> >>>> +APValue::LValuePathEntry::LValuePathEntry(BaseOrMemberType >>>> BaseOrMember) { >>>> + if (const Decl *D = BaseOrMember.getPointer()) >>>> + BaseOrMember.setPointer(D->getCanonicalDecl()); >>>> + Value = reinterpret_cast<uintptr_t>(BaseOrMember.getOpaqueValue()); >>>> +} >>>> + >>>> namespace { >>>> struct LVBase { >>>> APValue::LValueBase Base; >>>> @@ -113,14 +119,16 @@ APValue::LValueBase::operator bool () const { >>>> >>>> clang::APValue::LValueBase >>>> llvm::DenseMapInfo<clang::APValue::LValueBase>::getEmptyKey() { >>>> - return clang::APValue::LValueBase( >>>> - DenseMapInfo<const ValueDecl*>::getEmptyKey()); >>>> + clang::APValue::LValueBase B; >>>> + B.Ptr = DenseMapInfo<const ValueDecl*>::getEmptyKey(); >>>> + return B; >>>> } >>>> >>>> clang::APValue::LValueBase >>>> llvm::DenseMapInfo<clang::APValue::LValueBase>::getTombstoneKey() { >>>> - return clang::APValue::LValueBase( >>>> - DenseMapInfo<const ValueDecl*>::getTombstoneKey()); >>>> + clang::APValue::LValueBase B; >>>> + B.Ptr = DenseMapInfo<const ValueDecl*>::getTombstoneKey(); >>>> + return B; >>>> } >>>> >>>> namespace clang { >>>> @@ -773,8 +781,10 @@ void APValue::MakeMemberPointer(const ValueDecl >>>> *Member, bool IsDerivedMember, >>>> assert(isAbsent() && "Bad state change"); >>>> MemberPointerData *MPD = new ((void*)(char*)Data.buffer) >>>> MemberPointerData; >>>> Kind = MemberPointer; >>>> - MPD->MemberAndIsDerivedMember.setPointer(Member); >>>> + MPD->MemberAndIsDerivedMember.setPointer( >>>> + Member ? cast<ValueDecl>(Member->getCanonicalDecl()) : nullptr); >>>> MPD->MemberAndIsDerivedMember.setInt(IsDerivedMember); >>>> MPD->resizePath(Path.size()); >>>> - memcpy(MPD->getPath(), Path.data(), Path.size()*sizeof(const >>>> CXXRecordDecl*)); >>>> + for (unsigned I = 0; I != Path.size(); ++I) >>>> + MPD->getPath()[I] = Path[I]->getCanonicalDecl(); >>>> } >>>> >>>> diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp >>>> index 0ee1399d42df..c96450b8a377 100644 >>>> --- a/clang/lib/AST/Decl.cpp >>>> +++ b/clang/lib/AST/Decl.cpp >>>> @@ -4686,11 +4686,9 @@ char *Buffer = new (getASTContext(), 1) >>>> char[Name.size() + 1]; >>>> void ValueDecl::anchor() {} >>>> >>>> bool ValueDecl::isWeak() const { >>>> - for (const auto *I : attrs()) >>>> - if (isa<WeakAttr>(I) || isa<WeakRefAttr>(I)) >>>> - return true; >>>> - >>>> - return isWeakImported(); >>>> + auto *MostRecent = getMostRecentDecl(); >>>> + return MostRecent->hasAttr<WeakAttr>() || >>>> + MostRecent->hasAttr<WeakRefAttr>() || isWeakImported(); >>>> } >>>> >>>> void ImplicitParamDecl::anchor() {} >>>> >>>> diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp >>>> index f4314d0bd961..ab2b55c0762e 100644 >>>> --- a/clang/lib/AST/DeclBase.cpp >>>> +++ b/clang/lib/AST/DeclBase.cpp >>>> @@ -720,7 +720,7 @@ bool Decl::isWeakImported() const { >>>> if (!canBeWeakImported(IsDefinition)) >>>> return false; >>>> >>>> - for (const auto *A : attrs()) { >>>> + for (const auto *A : getMostRecentDecl()->attrs()) { >>>> if (isa<WeakImportAttr>(A)) >>>> return true; >>>> >>>> >>>> diff --git a/clang/lib/AST/ExprConstant.cpp >>>> b/clang/lib/AST/ExprConstant.cpp >>>> index 9bc4afd0b9f8..3bc649b96990 100644 >>>> --- a/clang/lib/AST/ExprConstant.cpp >>>> +++ b/clang/lib/AST/ExprConstant.cpp >>>> @@ -1978,18 +1978,11 @@ static bool HasSameBase(const LValue &A, const >>>> LValue &B) { >>>> return false; >>>> >>>> if (A.getLValueBase().getOpaqueValue() != >>>> - B.getLValueBase().getOpaqueValue()) { >>>> - const Decl *ADecl = GetLValueBaseDecl(A); >>>> - if (!ADecl) >>>> - return false; >>>> - const Decl *BDecl = GetLValueBaseDecl(B); >>>> - if (!BDecl || ADecl->getCanonicalDecl() != >>>> BDecl->getCanonicalDecl()) >>>> - return false; >>>> - } >>>> + B.getLValueBase().getOpaqueValue()) >>>> + return false; >>>> >>>> - return IsGlobalLValue(A.getLValueBase()) || >>>> - (A.getLValueCallIndex() == B.getLValueCallIndex() && >>>> - A.getLValueVersion() == B.getLValueVersion()); >>>> + return A.getLValueCallIndex() == B.getLValueCallIndex() && >>>> + A.getLValueVersion() == B.getLValueVersion(); >>>> } >>>> >>>> static void NoteLValueLocation(EvalInfo &Info, APValue::LValueBase >>>> Base) { >>>> @@ -3158,7 +3151,8 @@ static bool evaluateVarDeclInit(EvalInfo &Info, >>>> const Expr *E, >>>> >>>> // If we're currently evaluating the initializer of this >>>> declaration, use that >>>> // in-flight value. >>>> - if (Info.EvaluatingDecl.dyn_cast<const ValueDecl*>() == VD) { >>>> + if (declaresSameEntity(Info.EvaluatingDecl.dyn_cast<const ValueDecl >>>> *>(), >>>> + VD)) { >>>> Result = Info.EvaluatingDeclValue; >>>> return true; >>>> } >>>> >>>> diff --git a/clang/lib/CodeGen/CGExprConstant.cpp >>>> b/clang/lib/CodeGen/CGExprConstant.cpp >>>> index b0a37531dfe1..bff4a0c38af9 100644 >>>> --- a/clang/lib/CodeGen/CGExprConstant.cpp >>>> +++ b/clang/lib/CodeGen/CGExprConstant.cpp >>>> @@ -1877,6 +1877,10 @@ ConstantLValue >>>> ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) { >>>> // Handle values. >>>> if (const ValueDecl *D = base.dyn_cast<const ValueDecl*>()) { >>>> + // The constant always points to the canonical declaration. We >>>> want to look >>>> + // at properties of the most recent declaration at the point of >>>> emission. >>>> + D = cast<ValueDecl>(D->getMostRecentDecl()); >>>> + >>>> if (D->hasAttr<WeakRefAttr>()) >>>> return CGM.GetWeakRefReference(D).getPointer(); >>>> >>>> >>>> diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp >>>> b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp >>>> index 8d51dbde7177..3720b277af7a 100644 >>>> --- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp >>>> +++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp >>>> @@ -24,11 +24,10 @@ constexpr double &ni3; // expected-error >>>> {{declaration of reference variable 'ni >>>> >>>> constexpr int nc1 = i; // expected-error {{constexpr variable 'nc1' >>>> must be initialized by a constant expression}} expected-note {{read of >>>> non-const variable 'i' is not allowed in a constant expression}} >>>> constexpr C nc2 = C(); // expected-error {{cannot have non-literal >>>> type 'const C'}} >>>> -int &f(); // expected-note {{declared here}} >>>> +int &f(); // expected-note 2{{declared here}} >>>> constexpr int &nc3 = f(); // expected-error {{constexpr variable 'nc3' >>>> must be initialized by a constant expression}} expected-note >>>> {{non-constexpr function 'f' cannot be used in a constant expression}} >>>> constexpr int nc4(i); // expected-error {{constexpr variable 'nc4' >>>> must be initialized by a constant expression}} expected-note {{read of >>>> non-const variable 'i' is not allowed in a constant expression}} >>>> constexpr C nc5((C())); // expected-error {{cannot have non-literal >>>> type 'const C'}} >>>> -int &f(); // expected-note {{here}} >>>> constexpr int &nc6(f()); // expected-error {{constexpr variable 'nc6' >>>> must be initialized by a constant expression}} expected-note >>>> {{non-constexpr function 'f'}} >>>> >>>> struct pixel { >>>> >>>> diff --git a/clang/test/CodeGenCXX/weak-external.cpp >>>> b/clang/test/CodeGenCXX/weak-external.cpp >>>> index a2c53a59dcd5..433fb3c80624 100644 >>>> --- a/clang/test/CodeGenCXX/weak-external.cpp >>>> +++ b/clang/test/CodeGenCXX/weak-external.cpp >>>> @@ -64,3 +64,15 @@ class _LIBCPP_EXCEPTION_ABI runtime_error >>>> void dummysymbol() { >>>> throw(std::runtime_error("string")); >>>> } >>>> + >>>> +namespace not_weak_on_first { >>>> + int func(); >>>> + // CHECK: {{.*}} extern_weak {{.*}} @_ZN17not_weak_on_first4funcEv( >>>> + int func() __attribute__ ((weak)); >>>> + >>>> + typedef int (*FuncT)(); >>>> + >>>> + extern const FuncT table[] = { >>>> + func, >>>> + }; >>>> +} >>>> >>>> diff --git a/clang/test/OpenMP/ordered_messages.cpp >>>> b/clang/test/OpenMP/ordered_messages.cpp >>>> index f6b9dbd6d27f..8a3a86443eb8 100644 >>>> --- a/clang/test/OpenMP/ordered_messages.cpp >>>> +++ b/clang/test/OpenMP/ordered_messages.cpp >>>> @@ -16,6 +16,9 @@ void xxx(int argc) { >>>> } >>>> >>>> int foo(); >>>> +#if __cplusplus >= 201103L >>>> +// expected-note@-2 {{declared here}} >>>> +#endif >>>> >>>> template <class T> >>>> T foo() { >>>> @@ -176,7 +179,7 @@ T foo() { >>>> >>>> int foo() { >>>> #if __cplusplus >= 201103L >>>> -// expected-note@-2 2 {{declared here}} >>>> +// expected-note@-2 {{declared here}} >>>> #endif >>>> int k; >>>> #pragma omp for ordered >>>> >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> cfe-commits@lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>> >>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
