[PATCH] D15156: [ThinLTO] Appending linkage fixes

[Rafael Espíndola via llvm-commits]

Can this be tested with llvm-link instead of opt? LGTM with that.

On 2 December 2015 at 12:17, Teresa Johnson <tejohnson at google.com> wrote:
> tejohnson created this revision.
> tejohnson added reviewers: joker.eph, rafael.
> tejohnson added a subscriber: llvm-commits.
> Herald added a subscriber: joker.eph.
>
> Fix import from module with appending var, which cannot be imported. The
> first fix is to remove an overly-aggressive error check.
>
> The second fix is to deal with restructuring introduced to the module
> linker yesterday in r254418. The handling in linkGlobalValueProto for
> not linking in appending variables during function importing is coming
> too late, when we are materializing decls. Add the same check to
> linkIfNeeded. (Rafael, perhaps the checking in linkGlobalValueProto and
> linkIfNeeded should be commoned? Also, I'm concerned about the case
> where linkGlobalValueProto decides linkFromSrc==false and there is no
> existing dest GV. Previously we would skip linking for that source GV,
> but now we will create a decl in the dest module, as the early return
> when "!LinkFromSrc && !DGV" has been removed from linkGlobalValueProto
> due to the change in callsite. Are there other lurking issues?)
>
> Test by Mehdi Amini.
>
> http://reviews.llvm.org/D15156
>
> Files:
>   lib/Linker/LinkModules.cpp
>   test/Transforms/FunctionImport/Inputs/appending_global.ll
>   test/Transforms/FunctionImport/import_appending_global.ll
>
> Index: test/Transforms/FunctionImport/import_appending_global.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/FunctionImport/import_appending_global.ll
> @@ -0,0 +1,18 @@
> +; RUN: llvm-as -function-summary %s -o %t.bc
> +; RUN: llvm-as -function-summary %p/Inputs/appending_global.ll -o %t2.bc
> +; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
> +
> +; Do the import now
> +; RUN: opt -function-import -summary-file %t3.thinlto.bc %s -S | FileCheck %s
> +
> +; Ensure that global constructors (appending linkage) is not imported
> +; CHECK-NOT: @llvm.global_ctors
> +
> +
> +define i32 @main() {
> +entry:
> +  call void @foo()
> +  ret i32 0
> +}
> +
> +declare void @foo()
> Index: test/Transforms/FunctionImport/Inputs/appending_global.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/FunctionImport/Inputs/appending_global.ll
> @@ -0,0 +1,6 @@
> + at v = weak global i8 1
> + at llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @foo, i8* @v}]
> +
> +define void @foo() {
> +  ret void
> +}
> Index: lib/Linker/LinkModules.cpp
> ===================================================================
> --- lib/Linker/LinkModules.cpp
> +++ lib/Linker/LinkModules.cpp
> @@ -762,8 +762,10 @@
>      // It would be incorrect to import an appending linkage variable,
>      // since it would cause global constructors/destructors to be
>      // executed multiple times. This should have already been handled
> -    // by linkGlobalValueProto.
> -    llvm_unreachable("Cannot import appending linkage variable");
> +    // by linkGlobalValueProto, and we will assert in shouldLinkFromSource
> +    // if we try to import, so we simply return AppendingLinkage here
> +    // as this helper is called more widely in getLinkedToGlobal.
> +    return GlobalValue::AppendingLinkage;
>
>    case GlobalValue::InternalLinkage:
>    case GlobalValue::PrivateLinkage:
> @@ -1861,7 +1863,8 @@
>
>    if (!DGV && !shouldOverrideFromSrc() &&
>        (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
> -       GV.hasAvailableExternallyLinkage())) {
> +       GV.hasAvailableExternallyLinkage() ||
> +       (GV.hasAppendingLinkage() && isPerformingImport()))) {
>      return false;
>    }
>    MapValue(&GV, ValueMap, RF_MoveDistinctMDs, &TypeMap, &ValMaterializer);
>
>