[PATCH] D19527: LTOCodeGenerator: add linkonce to llvm.used when in "MustPreserve" set
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 26 09:55:45 PDT 2016
> On 2016-Apr-26, at 09:35, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>
> joker.eph marked 2 inline comments as done.
> joker.eph added a comment.
>
> Thanks for the comments.
>
>
> ================
> Comment at: lib/LTO/LTOCodeGenerator.cpp:394
> @@ +393,3 @@
> + if (ScopeRestrictionsDone)
> + return;
> +
> ----------------
> Yes, good catch. It turns out that the next variable declaration was unused, and was taken as body for the if block, which is why the code was still compiling...
>
>
>
> http://reviews.llvm.org/D19527
Any idea why this is only coming up now? It doesn't seem like a new bug...
I think llvm.compiler.used is more appropriate than llvm.used.
I'm also not sure adding to the llvm.compiler.used (or llvm.used) is the
right way to prevent linkonce/linkonce_odr functions from being
discarded. Should you just change their linkage to weak/weak_odr?
I.e., this seems like the moral equivalent of the following code in
gold-plugin.cpp:
--
case LDPR_PREVAILING_DEF:
Keep.push_back(GV);
// There is a non IR use, so we have to force optimizations to keep this.
switch (GV->getLinkage()) {
default:
break;
case GlobalValue::LinkOnceAnyLinkage:
GV->setLinkage(GlobalValue::WeakAnyLinkage);
break;
case GlobalValue::LinkOnceODRLinkage:
GV->setLinkage(GlobalValue::WeakODRLinkage);
break;
}
break;
--
Should we take the same approach?
> Index: test/tools/lto/hide-linkonce-odr.ll
> ===================================================================
> --- test/tools/lto/hide-linkonce-odr.ll
> +++ test/tools/lto/hide-linkonce-odr.ll
> @@ -1,9 +1,11 @@
> ; RUN: llvm-as %s -o %t.o
> -; RUN: %ld64 -lto_library %llvmshlibdir/libLTO.dylib -dylib -arch x86_64 -macosx_version_min 10.10.0 -lSystem -o %t.dylib %t.o -save-temps -undefined dynamic_lookup
> +; RUN: %ld64 -lto_library %llvmshlibdir/libLTO.dylib -dylib -arch x86_64 -macosx_version_min 10.10.0 -lSystem -o %t.dylib %t.o -save-temps -undefined dynamic_lookup -exported_symbol _c -exported_symbol _b
>
> ; RUN: llvm-dis %t.dylib.lto.opt.bc -o - | FileCheck --check-prefix=IR %s
> -; check that @a is still a linkonce_odr definition
> -; IR: define linkonce_odr void @a()
> +; check that @a is no longer a linkonce_odr definition
> +; IR-NOT: define linkonce_odr void @a()
> +; check that @b is appended in llvm.used
> +; IR: @llvm.used = appending global [1 x i8*] [i8* bitcast (void ()* @b to i8*)], section "llvm.metadata"
>
> ; RUN: llvm-nm %t.dylib | FileCheck --check-prefix=NM %s
> ; check that the linker can hide @a but not @b
> Index: lib/LTO/LTOCodeGenerator.cpp
> ===================================================================
> --- lib/LTO/LTOCodeGenerator.cpp
> +++ lib/LTO/LTOCodeGenerator.cpp
> @@ -348,14 +348,74 @@
> RelocModel, CodeModel::Default, CGOptLevel));
> }
>
> +// If a linkonce global is present in the MustPreserveSymbols, we need to make
> +// sure we honor this. To force the compiler to not drop it, we add it to the
> +// "llvm.used" global.
> +static void preserveDiscardableGV(
This this is preserving all the discardable GVs that should be preserved
(not just one), I think this should be called "preserveDiscardableGVs".
> + Module &TheModule,
> + const std::function<bool(const GlobalValue &)> &MustPreserveGV) {
I think you can use llvm::function_ref here since the function lifetime
isn't an issue.
Also, I usually name function objects the same way as other functions
(starting with a lower-case letter). I don't think our style guidelines
are clear one way or the other, but I thought I'd mention my tendency.
> + DenseSet<Constant *> UsedValuesSet;
You should use a SetVector here. Otherwise the output value of the
variable will depend on how pointers decide to hash on that particular
run.
> + if (GlobalVariable *LLVMUsed = TheModule.getGlobalVariable("llvm.used")) {
> + ConstantArray *Inits = cast<ConstantArray>(LLVMUsed->getInitializer());
> + for (auto &V : Inits->operands())
> + UsedValuesSet.insert(cast<Constant>(&V));
> + LLVMUsed->eraseFromParent();
This doesn't seem appropriate: you're *always* replacing the variable,
even if you're not going to change its contents.
> + }
> + llvm::Type *i8PTy = llvm::Type::getInt8PtrTy(TheModule.getContext());
> + auto mayPreserveGlobal = [&](GlobalValue &GV) {
"shouldPreserveGlobal"?
> + if (!GV.isDiscardableIfUnused() || GV.isDeclaration())
> + return;
> + if (!MustPreserveGV(GV))
> + return;
> + assert(!GV.hasAvailableExternallyLinkage() && !GV.hasInternalLinkage());
Couldn't it be that (in -flto=thin):
- The function is linkonce.
- It gets imported to another translation unit as
available_externally.
Or is that somehow excluded by logic elsewhere?
> + UsedValuesSet.insert(ConstantExpr::getBitCast(&GV, i8PTy));
> + };
> + for (auto &GV : TheModule)
> + mayPreserveGlobal(GV);
> + for (auto &GV : TheModule.globals())
> + mayPreserveGlobal(GV);
> + for (auto &GV : TheModule.aliases())
> + mayPreserveGlobal(GV);
> +
> + if (UsedValuesSet.empty())
> + return;
> + std::vector<Constant *> UsedValuesList(UsedValuesSet.begin(),
> + UsedValuesSet.end());
> +
> + llvm::ArrayType *ATy = llvm::ArrayType::get(i8PTy, UsedValuesList.size());
> + auto *LLVMUsed = new llvm::GlobalVariable(
> + TheModule, ATy, false, llvm::GlobalValue::AppendingLinkage,
> + llvm::ConstantArray::get(ATy, UsedValuesList), "llvm.used");
> + LLVMUsed->setSection("llvm.metadata");
> +}
> +
> void LTOCodeGenerator::applyScopeRestrictions() {
> - if (ScopeRestrictionsDone || !ShouldInternalize)
> + if (ScopeRestrictionsDone)
> + return;
> +
> + // Declare a callback for the internalize pass that will ask for every
> + // candidate GlobalValue if it can be internalized or not.
> + SmallString<64> MangledName;
> + auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
> + // Need to mangle the GV as the "MustPreserveSymbols" StringSet is filled
> + // with the linker supplied name, which on Darwin includes a leading
> + // underscore.
> + MangledName.clear();
> + MangledName.reserve(GV.getName().size() + 1);
> + Mangler::getNameWithPrefix(MangledName, GV.getName(),
> + MergedModule->getDataLayout());
> + return MustPreserveSymbols.count(MangledName);
> + };
> +
> + // Preserve linkonce value on linker request
> + preserveDiscardableGV(*MergedModule, MustPreserveGV);
> +
> + if (!ShouldInternalize)
> return;
>
> if (ShouldRestoreGlobalsLinkage) {
> // Record the linkage type of non-local symbols so they can be restored
> - // prior
> - // to module splitting.
> + // prior to module splitting.
> auto RecordLinkage = [&](const GlobalValue &GV) {
> if (!GV.hasAvailableExternallyLinkage() && !GV.hasLocalLinkage() &&
> GV.hasName())
> @@ -373,21 +433,6 @@
> // symbols referenced from asm
> UpdateCompilerUsed(*MergedModule, *TargetMach, AsmUndefinedRefs);
^ If we stick with the "used" way of doing it, should this get merged
with this code somehow?
>
> - // Declare a callback for the internalize pass that will ask for every
> - // candidate GlobalValue if it can be internalized or not.
> - Mangler Mangler;
> - SmallString<64> MangledName;
> - auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
> - // Need to mangle the GV as the "MustPreserveSymbols" StringSet is filled
> - // with the linker supplied name, which on Darwin includes a leading
> - // underscore.
> - MangledName.clear();
> - MangledName.reserve(GV.getName().size() + 1);
> - Mangler::getNameWithPrefix(MangledName, GV.getName(),
> - MergedModule->getDataLayout());
> - return MustPreserveSymbols.count(MangledName);
> - };
> -
> internalizeModule(*MergedModule, MustPreserveGV);
>
> ScopeRestrictionsDone = true;
>
More information about the llvm-commits
mailing list