[clang] 9dcd96f - Canonicalize declaration pointers when forming APValues.
Arthur Eubanks via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 12 11:07:54 PDT 2020
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 <richard at 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 <richard at metafoo.co.uk> wrote:
>
>> On Fri, 9 Oct 2020 at 19:12, Arthur Eubanks <aeubanks at 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 at 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 at -2 {{declared here}}
>>>> +#endif
>>>>
>>>> template <class T>
>>>> T foo() {
>>>> @@ -176,7 +179,7 @@ T foo() {
>>>>
>>>> int foo() {
>>>> #if __cplusplus >= 201103L
>>>> -// expected-note at -2 2 {{declared here}}
>>>> +// expected-note at -2 {{declared here}}
>>>> #endif
>>>> int k;
>>>> #pragma omp for ordered
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201012/61dccdf7/attachment-0001.html>
More information about the cfe-commits
mailing list