<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 29, 2017 at 2:46 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think I understand *a* problem, but it is not that ThinLTO specific.<br>
<br>
Consider the IR<br>
<br>
$foo = comdat any<br>
@foo = weak_odr global i32 42, comdat($foo)<br>
@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @bar, i8* bitcast (i32* @foo to i8*) }]<br>
define internal void @bar() comdat($foo) {<br>
ret void<br>
}<br>
<br>
Compiling it to a .o produces the expected result: everything in a<br>
comdat:<br>
<br>
COMDAT group section [ 3] `.group' [foo] contains 4 sections:<br>
[Index] Name<br>
[ 4] .text.bar<br>
[ 5] .data.foo<br>
[ 6] .init_array<br>
[ 7] .rela.init_array<br>
<br>
But link the .bc and the .o file with<br>
<br>
ld.lld test.o test.bc -o t -save-temps<br>
<br>
And we get<br>
<br>
$foo = comdat any<br>
@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @bar, i8* bitcast (i32* @foo to i8*) }]<br>
@foo = available_externally global i32 42<br>
define internal void @bar() comdat($foo) {<br>
ret void<br>
}<br>
<br>
That is invalid. The symbol @bar is not visible to the linker, so there<br>
is nothing it can do about it.<br>
<br>
As you point out it is not normally a problem because the linker will<br>
finish the job and ignore this incomplete comdat, but not in ThinLTO mode.<br>
<br>
But with your patch we would end up with<br>
<br>
@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @bar, i8* bitcast (i32* @foo to i8*) }]<br>
@foo = available_externally global i32 42<br>
define internal void @bar() {<br>
ret void<br>
}<br>
<br>
no?<br></blockquote><div><br></div><div>Right</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
That solves the comdat issue, but now @bar will be linked in even with<br>
regular LTO.<br></blockquote><div><br></div><div>Yes that's a good point.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I guess that is OK, for now, but we should try to improve it.<br>
<br>
Normally a _odr symbol doesn't refer to local ones. In this case it<br>
indirectly does:<br>
<br>
* @foo causes its entry in llvm.global_ctors to stay alive.<br>
* The entry causes @bar to stay alive.<br>
<br>
I think that in the only case where something like this happens in<br>
practice.<br>
<br>
So LGTM, but please in a followup patch change the code handling<br>
llvm.global_ctors to drop an entry if the corresponding global is<br>
changed to available_externally.<br></blockquote><div><br></div><div>Ok will get this in now to unblock David, but will do the followup as soon as I can.</div><div><br></div><div>Thanks,</div><div>Teresa</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
<div class="HOEnZb"><div class="h5"><br>
Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>> writes:<br>
<br>
> On Thu, Jun 29, 2017 at 12:49 PM, Rafael Avila de Espindola <<br>
> <a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
>> I am probably missing something.<br>
>><br>
>> But if a symbol in a comdat is prevailing and another one is not, that is<br>
>> a bug in the linker, no?<br>
>><br>
><br>
> It's not that we had a symbol in comdat that was prevailing and another was<br>
> not, presumably they were all not prevailing. But we only change the<br>
> linkage type for nonprevailing weak/linkonce (and were then necessarily<br>
> removing them from the comdat as it's illegal to have an<br>
> available_externally in a comdat). This is just handling the rest of the<br>
> symbols in the comdat (e.g. internals) that were being left in the comdat<br>
> resulting in an incomplete comdat. This only causes an issue when this<br>
> happens in the regular LTO partitions, since the resulting native .o file<br>
> is later first in the final native link order (and thus we are in danger of<br>
> selecting the incomplete comdat that was not originally selected by the<br>
> linker). I suppose I could change this to simply removal all non-prevailing<br>
> symbols from comdats in addRegularLTO (rather than just keeping track of<br>
> which had a weak/linkonce removed from the comdat and fixing them up later).<br>
><br>
> (As an aside I think my concocted example of an external variable in the<br>
> comdat needing to be dropped to available_externally might be bogus as I<br>
> had to mark both copies as prevailing to prevent them from being<br>
> internalized in the test case. But in any case the original issue was with<br>
> the internal variable in the comdat).<br>
><br>
> Teresa<br>
><br>
><br>
>> Cheers,<br>
>> Rafael<br>
>><br>
>> Teresa Johnson via Phabricator via llvm-commits<br>
>> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
>><br>
>> > tejohnson created this revision.<br>
>> > Herald added subscribers: inglorion, mehdi_amini.<br>
>> ><br>
>> > When linking a regular LTO module, if it has any non-prevailing values<br>
>> > (dropped to available_externally) in comdats, we need to do more than<br>
>> > just remove those values from their comdat. We also remove all values<br>
>> > from that comdat, so as to avoid leaving an incomplete comdat.<br>
>> ><br>
>> > This is necessary in case we are compiling in mixed regular and ThinLTO<br>
>> > mode, since the resulting regularLTO native object is always linked into<br>
>> > the final binary first. We need to prevent the linker from selecting an<br>
>> > incomplete comdat that was not the prevailing copy.<br>
>> ><br>
>> > Fixes PR32980.<br>
>> ><br>
>> ><br>
>> > <a href="https://reviews.llvm.org/D34803" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34803</a><br>
>> ><br>
>> > Files:<br>
>> > lib/LTO/LTO.cpp<br>
>> > test/LTO/Resolution/X86/<wbr>Inputs/comdat-mixed-lto.ll<br>
>> > test/LTO/Resolution/X86/<wbr>comdat-mixed-lto.ll<br>
>> ><br>
>> > Index: test/LTO/Resolution/X86/<wbr>comdat-mixed-lto.ll<br>
>> > ==============================<wbr>==============================<wbr>=======<br>
>> > --- /dev/null<br>
>> > +++ test/LTO/Resolution/X86/<wbr>comdat-mixed-lto.ll<br>
>> > @@ -0,0 +1,42 @@<br>
>> > +; Test of comdat handling with mixed thinlto and regular lto<br>
>> compilation.<br>
>> > +<br>
>> > +; This module is compiled with ThinLTO<br>
>> > +; RUN: opt -module-summary -o %t1.o %s<br>
>> > +; Input module compiled for regular LTO<br>
>> > +; RUN: opt -o %t2.o %p/Inputs/comdat-mixed-lto.ll<br>
>> > +<br>
>> > +; The copy of C from this module is prevailing. The copy of C from the<br>
>> > +; regular LTO module is not prevailing, and will be dropped to<br>
>> > +; available_externally.<br>
>> > +; RUN: llvm-lto2 run -r=%t1.o,C,pl -r=%t2.o,C,l<br>
>> -r=%t2.o,testglobfunc,lxp -r=%t1.o,testglobfunc,lx -o %t3 %t1.o %t2.o<br>
>> -save-temps<br>
>> > +<br>
>> > +; The Input module (regular LTO) is %t3.0. Check to make sure that we<br>
>> removed<br>
>> > +; __cxx_global_var_init and testglobfunc from comdat. Also check to<br>
>> ensure<br>
>> > +; that testglobfunc was dropped to available_externally. Otherwise we<br>
>> would<br>
>> > +; have linker multiply defined errors as it is no longer in a comdat and<br>
>> > +; would clash with the copy from this module.<br>
>> > +; RUN: llvm-dis %t3.0.0.preopt.bc -o - | FileCheck %s<br>
>> > +; CHECK: define internal void @__cxx_global_var_init() section<br>
>> ".text.startup" {<br>
>> > +; CHECK: define available_externally void @testglobfunc() section<br>
>> ".text.startup" {<br>
>> > +<br>
>> > +; ModuleID = 'comdat-mixed-lto.o'<br>
>> > +source_filename = "comdat-mixed-lto.cpp"<br>
>> > +target datalayout = "e-m:e-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
>> > +target triple = "x86_64-unknown-linux-gnu"<br>
>> > +<br>
>> > +%"class.Test::ptr" = type { i32 }<br>
>> > +<br>
>> > +$C = comdat any<br>
>> > +<br>
>> > +@C = linkonce_odr global %"class.Test::ptr" zeroinitializer, comdat,<br>
>> align 4<br>
>> > +@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{<br>
>> i32, void ()*, i8* } { i32 65535, void ()* @__cxx_global_var_init, i8*<br>
>> bitcast (%"class.Test::ptr"* @C to i8*) }]<br>
>> > +define void @testglobfunc() #1 section ".text.startup" comdat($C) {<br>
>> > +entry:<br>
>> > + ret void<br>
>> > +}<br>
>> > +<br>
>> > +; Function Attrs: noinline uwtable<br>
>> > +define internal void @__cxx_global_var_init() #1 section<br>
>> ".text.startup" comdat($C) {<br>
>> > +entry:<br>
>> > + ret void<br>
>> > +}<br>
>> > Index: test/LTO/Resolution/X86/<wbr>Inputs/comdat-mixed-lto.ll<br>
>> > ==============================<wbr>==============================<wbr>=======<br>
>> > --- /dev/null<br>
>> > +++ test/LTO/Resolution/X86/<wbr>Inputs/comdat-mixed-lto.ll<br>
>> > @@ -0,0 +1,23 @@<br>
>> > +; ModuleID = 'comdat-mixed-lto1.o'<br>
>> > +source_filename = "comdat-mixed-lto1.cpp"<br>
>> > +target datalayout = "e-m:e-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
>> > +target triple = "x86_64-unknown-linux-gnu"<br>
>> > +<br>
>> > +%"class.Test::ptr" = type { i32 }<br>
>> > +<br>
>> > +$C = comdat any<br>
>> > +<br>
>> > +@C = linkonce_odr global %"class.Test::ptr" zeroinitializer, comdat,<br>
>> align 4<br>
>> > +@llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{<br>
>> i32, void ()*, i8* } { i32 65535, void ()* @__cxx_global_var_init, i8*<br>
>> bitcast (%"class.Test::ptr"* @C to i8*) }]<br>
>> > +<br>
>> > +define void @testglobfunc() #1 section ".text.startup" comdat($C) {<br>
>> > +entry:<br>
>> > + ret void<br>
>> > +}<br>
>> > +<br>
>> > +; Function Attrs: noinline uwtable<br>
>> > +define internal void @__cxx_global_var_init() #1 section<br>
>> ".text.startup" comdat($C) {<br>
>> > +entry:<br>
>> > + store i32 0, i32* getelementptr inbounds (%"class.Test::ptr",<br>
>> %"class.Test::ptr"* @C, i32 0, i32 0), align 4<br>
>> > + ret void<br>
>> > +}<br>
>> > Index: lib/LTO/LTO.cpp<br>
>> > ==============================<wbr>==============================<wbr>=======<br>
>> > --- lib/LTO/LTO.cpp<br>
>> > +++ lib/LTO/LTO.cpp<br>
>> > @@ -472,6 +472,36 @@<br>
>> > return Error::success();<br>
>> > }<br>
>> ><br>
>> > +// Checks whether the given global value is in a non-prevailing comdat<br>
>> > +// (comdat containing values the linker indicated were not prevailing,<br>
>> > +// which we then dropped to available_externally), and if so, removes<br>
>> > +// it from the comdat. This is called for all global values to ensure<br>
>> the<br>
>> > +// comdat is empty rather than leaving an incomplete comdat. It is<br>
>> needed for<br>
>> > +// regular LTO modules, in case we are in a mixed-LTO mode (both regular<br>
>> > +// and thin LTO modules) compilation. Since the regular LTO module will<br>
>> be<br>
>> > +// linked first in the final native link, we want to make sure the<br>
>> linker<br>
>> > +// doesn't select any of these incomplete comdats that would be left<br>
>> > +// in the regular LTO module without this cleanup.<br>
>> > +static void<br>
>> > +handleNonPrevailingComdat(<wbr>GlobalValue &GV,<br>
>> > + std::set<const Comdat *><br>
>> &NonPrevailingComdats) {<br>
>> > + Comdat *C = GV.getComdat();<br>
>> > + if (!C)<br>
>> > + return;<br>
>> > +<br>
>> > + if (!NonPrevailingComdats.count(<wbr>C))<br>
>> > + return;<br>
>> > +<br>
>> > + // Additionally need to drop externally visible global values from<br>
>> the comdat<br>
>> > + // to available_externally, so that there aren't multiply defined<br>
>> linker<br>
>> > + // errors.<br>
>> > + if (!GV.hasLocalLinkage())<br>
>> > + GV.setLinkage(GlobalValue::<wbr>AvailableExternallyLinkage);<br>
>> > +<br>
>> > + if (auto GO = dyn_cast<GlobalObject>(&GV))<br>
>> > + GO->setComdat(nullptr);<br>
>> > +}<br>
>> > +<br>
>> > // Add a regular LTO object to the link.<br>
>> > // The resulting module needs to be linked into the combined LTO module<br>
>> with<br>
>> > // linkRegularLTO.<br>
>> > @@ -523,6 +553,7 @@<br>
>> > };<br>
>> > Skip();<br>
>> ><br>
>> > + std::set<const Comdat *> NonPrevailingComdats;<br>
>> > for (const InputFile::Symbol &Sym : Syms) {<br>
>> > assert(ResI != ResE);<br>
>> > SymbolResolution Res = *ResI++;<br>
>> > @@ -557,6 +588,8 @@<br>
>> > // module (in linkRegularLTO), based on whether it is undefined.<br>
>> > Mod.Keep.push_back(GV);<br>
>> > GV->setLinkage(GlobalValue::<wbr>AvailableExternallyLinkage);<br>
>> > + if (GV->hasComdat())<br>
>> > + NonPrevailingComdats.insert(<wbr>GV->getComdat());<br>
>> > cast<GlobalObject>(GV)-><wbr>setComdat(nullptr);<br>
>> > }<br>
>> > }<br>
>> > @@ -574,6 +607,9 @@<br>
>> ><br>
>> > // FIXME: use proposed local attribute for<br>
>> FinalDefinitionInLinkageUnit.<br>
>> > }<br>
>> > + if (!M.getComdatSymbolTable().<wbr>empty())<br>
>> > + for (GlobalValue &GV : M.global_values())<br>
>> > + handleNonPrevailingComdat(GV, NonPrevailingComdats);<br>
>> > assert(MsymI == MsymE);<br>
>> > return std::move(Mod);<br>
>> > }<br>
>> > ______________________________<wbr>_________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
>><br>
><br>
><br>
><br>
> --<br>
> Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com">tejohnson@google.com</a> | <a href="tel:408-460-2413" value="+14084602413">408-460-2413</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>