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