r313957 - Closure types have no name (and can't have a typedef name for linkage

[Richard Smith via cfe-commits]

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%
> 20Linux%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.

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.


> 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.
>> cpp?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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171002/de30ac77/attachment.html>