[PATCH] D34803: [LTO] Remove values from non-prevailing comdats
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 13:28:32 PDT 2017
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170629/e2b76bc5/attachment.html>
More information about the llvm-commits
mailing list