<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 2, 2017 at 7:02 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="m_-4149405043404066311gmail-">On 2 October 2017 at 17:10, Peter Collingbourne via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Looks like this caused PR34811, which caused a link error on a Chromium bot:<div><div><div><a href="https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/7081" target="_blank">https://build.chromium.org/p/c<wbr>hromium.fyi/builders/CFI%20Lin<wbr>ux%20ToT/builds/7081</a><br></div></div></div><div><br></div><div>The link error might be caused by an unrelated LTO bug, but this bug does seem real.</div></div></blockquote><div><br></div></span><div>Agreed. This turned out to be a pre-existing bug that we hit a lot more often after this change. Fixed in r314754.</div></div></div></div></blockquote><div><br></div><div>Thanks for the quick fix!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>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.</div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>Peter</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="m_-4149405043404066311gmail-h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Peter</div></div><div class="gmail_extra"><div><div class="m_-4149405043404066311gmail-m_-2378237854668050119gmail-h5"><br><div class="gmail_quote">On Thu, Sep 21, 2017 at 9:33 PM, Richard Smith via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: rsmith<br>
Date: Thu Sep 21 21:33:20 2017<br>
New Revision: 313957<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=313957&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=313957&view=rev</a><br>
Log:<br>
Closure types have no name (and can't have a typedef name for linkage<br>
purposes), so they never formally have linkage.<br>
<br>
Modified:<br>
cfe/trunk/lib/AST/Decl.cpp<br>
cfe/trunk/test/CXX/basic/basic<wbr>.link/p8.cpp<br>
<br>
Modified: cfe/trunk/lib/AST/Decl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=313957&r1=313956&r2=313957&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/lib/AST/Decl.c<wbr>pp?rev=313957&r1=313956&r2=313<wbr>957&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/AST/Decl.cpp (original)<br>
+++ cfe/trunk/lib/AST/Decl.cpp Thu Sep 21 21:33:20 2017<br>
@@ -1104,24 +1104,25 @@ LinkageInfo LinkageComputer::getLVForClo<br>
else<br>
Owner = cast<NamedDecl>(ContextDecl);<br>
<br>
- // FIXME: If there is no owner, the closure should have no linkage.<br>
if (!Owner)<br>
- return LinkageInfo::external();<br>
+ return LinkageInfo::none();<br>
<br>
// If the owner has a deduced type, we need to skip querying the linkage and<br>
// visibility of that type, because it might involve this closure type. The<br>
// only effect of this is that we might give a lambda VisibleNoLinkage rather<br>
// than NoLinkage when we don't strictly need to, which is benign.<br>
auto *VD = dyn_cast<VarDecl>(Owner);<br>
- LinkageInfo OwnerLinkage =<br>
+ LinkageInfo OwnerLV =<br>
VD && VD->getType()->getContainedDed<wbr>ucedType()<br>
? computeLVForDecl(Owner, computation, /*IgnoreVarTypeLinkage*/true)<br>
: getLVForDecl(Owner, computation);<br>
<br>
- // FIXME: This is wrong. A lambda never formally has linkage; if this<br>
- // calculation determines a lambda has external linkage, it should be<br>
- // downgraded to VisibleNoLinkage.<br>
- return OwnerLinkage;<br>
+ // A lambda never formally has linkage. But if the owner is externally<br>
+ // visible, then the lambda is too. We apply the same rules to blocks.<br>
+ if (!isExternallyVisible(OwnerLV.<wbr>getLinkage()))<br>
+ return LinkageInfo::none();<br>
+ return LinkageInfo(VisibleNoLinkage, OwnerLV.getVisibility(),<br>
+ OwnerLV.isVisibilityExplicit(<wbr>));<br>
}<br>
<br>
LinkageInfo LinkageComputer::getLVForLocal<wbr>Decl(const NamedDecl *D,<br>
<br>
Modified: cfe/trunk/test/CXX/basic/basic<wbr>.link/p8.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/basic/basic.link/p8.cpp?rev=313957&r1=313956&r2=313957&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/CXX/basic<wbr>/basic.link/p8.cpp?rev=313957&<wbr>r1=313956&r2=313957&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/CXX/basic/basic<wbr>.link/p8.cpp (original)<br>
+++ cfe/trunk/test/CXX/basic/basic<wbr>.link/p8.cpp Thu Sep 21 21:33:20 2017<br>
@@ -14,7 +14,7 @@ typedef decltype(f()) NoLinkage3;<br>
inline auto g() { return [] {}; }<br>
typedef decltype(g()) VisibleNoLinkage1;<br>
inline auto y = [] {};<br>
-typedef decltype(x) VisibleNoLinkage2;<br>
+typedef decltype(y) VisibleNoLinkage2;<br>
inline auto h() { struct {} x; return x; }<br>
typedef decltype(h()) VisibleNoLinkage3;<br>
<br>
@@ -42,19 +42,12 @@ void use_no_linkage() {<br>
no_linkage3(); // expected-note {{used here}}<br>
}<br>
<br>
-// FIXME: This should emit an extension warning. It does not because we<br>
-// incorrectly give the lambda external linkage.<br>
-extern VisibleNoLinkage1 visible_no_linkage1();<br>
-<br>
-// FIXME: We should accept this as an extension. We don't because we<br>
-// incorrectly give the lambda no linkage instead of "VisibleNoLinkage".<br>
-extern VisibleNoLinkage2 visible_no_linkage2(); // expected-error {{used but not defined}}<br>
-<br>
-// This case is correctly accepted as an extension.<br>
+extern VisibleNoLinkage1 visible_no_linkage1(); // expected-warning {{ISO C++ requires a definition}}<br>
+extern VisibleNoLinkage2 visible_no_linkage2(); // expected-warning {{ISO C++ requires a definition}}<br>
extern VisibleNoLinkage3 visible_no_linkage3(); // expected-warning {{ISO C++ requires a definition}}<br>
<br>
void use_visible_no_linkage() {<br>
- visible_no_linkage1();<br>
+ visible_no_linkage1(); // expected-note {{used here}}<br>
visible_no_linkage2(); // expected-note {{used here}}<br>
visible_no_linkage3(); // expected-note {{used here}}<br>
}<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div></div></div><span class="m_-4149405043404066311gmail-m_-2378237854668050119gmail-HOEnZb"><font color="#888888">-- <br><div class="m_-4149405043404066311gmail-m_-2378237854668050119gmail-m_-5352921775338116957gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</font></span></div>
<br>______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_-4149405043404066311gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>