r313957 - Closure types have no name (and can't have a typedef name for linkage
Peter Collingbourne via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 2 19:33:38 PDT 2017
On Mon, Oct 2, 2017 at 7:02 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On 2 October 2017 at 17:10, Peter Collingbourne via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Looks like this caused PR34811, which caused a link error on a Chromium
>> bot:
>> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Lin
>> ux%20ToT/builds/7081
>>
>> The link error might be caused by an unrelated LTO bug, but this bug does
>> seem real.
>>
>
> Agreed. This turned out to be a pre-existing bug that we hit a lot more
> often after this change. Fixed in r314754.
>
Thanks for the quick fix!
> That said, this bug would only cause declarations to have default
> visibility when they should have some other visibility (it should only
> cause too much visibility, not too little), and it's not obvious to me how
> that could result in the link error on the Chromium bot.
>
The link error is probably an LTO bug. If a symbol has default visibility
and we are linking a DSO, the symbol is treated by a live root by the
linker. LTO has a mechanism for dead stripping of unused symbol
definitions, and I suspect that this mechanism isn't handling these symbols
correctly (perhaps because they are linkonce_odr?) and stripping
definitions that turn out to be needed for the final link.
Peter
>
>
>> Peter
>>
>> On Thu, Sep 21, 2017 at 9:33 PM, Richard Smith via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> Author: rsmith
>>> Date: Thu Sep 21 21:33:20 2017
>>> New Revision: 313957
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=313957&view=rev
>>> Log:
>>> Closure types have no name (and can't have a typedef name for linkage
>>> purposes), so they never formally have linkage.
>>>
>>> Modified:
>>> cfe/trunk/lib/AST/Decl.cpp
>>> cfe/trunk/test/CXX/basic/basic.link/p8.cpp
>>>
>>> Modified: cfe/trunk/lib/AST/Decl.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.c
>>> pp?rev=313957&r1=313956&r2=313957&view=diff
>>> ============================================================
>>> ==================
>>> --- cfe/trunk/lib/AST/Decl.cpp (original)
>>> +++ cfe/trunk/lib/AST/Decl.cpp Thu Sep 21 21:33:20 2017
>>> @@ -1104,24 +1104,25 @@ LinkageInfo LinkageComputer::getLVForClo
>>> else
>>> Owner = cast<NamedDecl>(ContextDecl);
>>>
>>> - // FIXME: If there is no owner, the closure should have no linkage.
>>> if (!Owner)
>>> - return LinkageInfo::external();
>>> + return LinkageInfo::none();
>>>
>>> // If the owner has a deduced type, we need to skip querying the
>>> linkage and
>>> // visibility of that type, because it might involve this closure
>>> type. The
>>> // only effect of this is that we might give a lambda
>>> VisibleNoLinkage rather
>>> // than NoLinkage when we don't strictly need to, which is benign.
>>> auto *VD = dyn_cast<VarDecl>(Owner);
>>> - LinkageInfo OwnerLinkage =
>>> + LinkageInfo OwnerLV =
>>> VD && VD->getType()->getContainedDeducedType()
>>> ? computeLVForDecl(Owner, computation,
>>> /*IgnoreVarTypeLinkage*/true)
>>> : getLVForDecl(Owner, computation);
>>>
>>> - // FIXME: This is wrong. A lambda never formally has linkage; if this
>>> - // calculation determines a lambda has external linkage, it should be
>>> - // downgraded to VisibleNoLinkage.
>>> - return OwnerLinkage;
>>> + // A lambda never formally has linkage. But if the owner is externally
>>> + // visible, then the lambda is too. We apply the same rules to blocks.
>>> + if (!isExternallyVisible(OwnerLV.getLinkage()))
>>> + return LinkageInfo::none();
>>> + return LinkageInfo(VisibleNoLinkage, OwnerLV.getVisibility(),
>>> + OwnerLV.isVisibilityExplicit());
>>> }
>>>
>>> LinkageInfo LinkageComputer::getLVForLocalDecl(const NamedDecl *D,
>>>
>>> Modified: cfe/trunk/test/CXX/basic/basic.link/p8.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/basic
>>> /basic.link/p8.cpp?rev=313957&r1=313956&r2=313957&view=diff
>>> ============================================================
>>> ==================
>>> --- cfe/trunk/test/CXX/basic/basic.link/p8.cpp (original)
>>> +++ cfe/trunk/test/CXX/basic/basic.link/p8.cpp Thu Sep 21 21:33:20 2017
>>> @@ -14,7 +14,7 @@ typedef decltype(f()) NoLinkage3;
>>> inline auto g() { return [] {}; }
>>> typedef decltype(g()) VisibleNoLinkage1;
>>> inline auto y = [] {};
>>> -typedef decltype(x) VisibleNoLinkage2;
>>> +typedef decltype(y) VisibleNoLinkage2;
>>> inline auto h() { struct {} x; return x; }
>>> typedef decltype(h()) VisibleNoLinkage3;
>>>
>>> @@ -42,19 +42,12 @@ void use_no_linkage() {
>>> no_linkage3(); // expected-note {{used here}}
>>> }
>>>
>>> -// FIXME: This should emit an extension warning. It does not because we
>>> -// incorrectly give the lambda external linkage.
>>> -extern VisibleNoLinkage1 visible_no_linkage1();
>>> -
>>> -// FIXME: We should accept this as an extension. We don't because we
>>> -// incorrectly give the lambda no linkage instead of "VisibleNoLinkage".
>>> -extern VisibleNoLinkage2 visible_no_linkage2(); // expected-error
>>> {{used but not defined}}
>>> -
>>> -// This case is correctly accepted as an extension.
>>> +extern VisibleNoLinkage1 visible_no_linkage1(); // expected-warning
>>> {{ISO C++ requires a definition}}
>>> +extern VisibleNoLinkage2 visible_no_linkage2(); // expected-warning
>>> {{ISO C++ requires a definition}}
>>> extern VisibleNoLinkage3 visible_no_linkage3(); // expected-warning
>>> {{ISO C++ requires a definition}}
>>>
>>> void use_visible_no_linkage() {
>>> - visible_no_linkage1();
>>> + visible_no_linkage1(); // expected-note {{used here}}
>>> visible_no_linkage2(); // expected-note {{used here}}
>>> visible_no_linkage3(); // expected-note {{used here}}
>>> }
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>>
>> --
>> --
>> Peter
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
--
--
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171002/e08a15b1/attachment.html>
More information about the cfe-commits
mailing list