[PATCH] D34803: [LTO] Remove values from non-prevailing comdats
Rafael Avila de Espindola via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 14:46:38 PDT 2017
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?
That solves the comdat issue, but now @bar will be linked in even with
regular LTO.
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.
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
More information about the llvm-commits
mailing list