On Mon, Oct 2, 2017 at 7:02 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On 2 October 2017 at 17:10, Peter Collingbourne via cfe-commits < > cfe-commits@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@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@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> >> >> >> -- >> -- >> Peter >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> > -- -- Peter
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits