[PATCH] D34450: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals (release_40)

[Benoit Belley via Phabricator via llvm-commits]

belleyb added inline comments.


================
Comment at: lib/Linker/LinkModules.cpp:358
+    // Always import variables with appending linkage.
+    if (!GV.hasAppendingLinkage()) {
+      // Don't import globals unless they are referenced by the destination
----------------
This extra checks at line 357 and 586 are the fix. Without it, the tests below are failing.

The rest of the changes around is only to make the code more readable, allowing me to better document the intent,


================
Comment at: test/Linker/only-needed-compiler-used.ll:8
+
+
+; CHECK-DAG:          @llvm.compiler.used = appending global [2 x i8*] [i8* @used1, i8* bitcast (i32* @used2 to i8*)], section "llvm.metadata"
----------------
The linker emits the references to the global variables in a different ordering when the `--only-needed` is used. This explains the use of the DAG FileCheck option below.



================
Comment at: test/Linker/only-needed-ctors2.ll:4
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-ctors.src.a.ll %p/Inputs/only-needed-ctors.src.b.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+
+; Empty destination module!
----------------
Note that the test case requires at one of the `-internalize` or `-only-needed` flag. Else, a link error is generated about `func1` being defined twice. This explains why this particular test case has only 3 runs, instead of 4, like most of the other ones.


================
Comment at: test/Linker/only-needed-ctors4.ll:4
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-ctors.src.a.ll %p/Inputs/only-needed-ctors.src.b.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+
+; Destination module:
----------------
Note that the test case requires at one of the `-internalize` or `-only-needed` flag. Else, a link error is generated about `func1` being defined twice. This explains why this particular test case has only 3 runs, instead of 4, like most of the other ones.


================
Comment at: test/Linker/only-needed-dtors2.ll:4
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-dtors.src.a.ll %p/Inputs/only-needed-dtors.src.b.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+
+; Empty destination module!
----------------
Note that the test case requires at one of the `-internalize` or `-only-needed` flag. Else, a link error is generated about `func1` being defined twice. This explains why this particular test case has only 3 runs, instead of 4, like most of the other ones.


================
Comment at: test/Linker/only-needed-dtors4.ll:4
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-dtors.src.a.ll %p/Inputs/only-needed-dtors.src.b.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+
+; Destination module:
----------------
Note that the test case requires at one of the `-internalize` or `-only-needed` flag. Else, a link error is generated about `func1` being defined twice. This explains why this particular test case has only 3 runs, instead of 4, like most of the other ones.


================
Comment at: test/Linker/only-needed-used.ll:8
+
+
+; CHECK-DAG:          @llvm.used = appending global [2 x i8*] [i8* @used1, i8* bitcast (i32* @used2 to i8*)], section "llvm.metadata"
----------------
The linker emits the references to the global variables in a different ordering when the `--only-needed` is used. This explains the use of the DAG FileCheck option below.




================
Comment at: test/Linker/only-needed-used.ll:10
+; CHECK-DAG:          @llvm.used = appending global [2 x i8*] [i8* @used1, i8* bitcast (i32* @used2 to i8*)], section "llvm.metadata"
+; INTERNALIZE-DAG:    @used1 = internal global i8 4
+; NO-INTERNALIZE-DAG: @used1 = global i8 4
----------------
This test gives a different result in the release_40 branch than it does in trunk (5.0). I don't know which is correct. I just noticed the difference.

In trunk, the `used` variables always have `global` linkage and never `internal global` linkage, even when the `-internalize` flag is used. See: https://reviews.llvm.org/D34448



https://reviews.llvm.org/D34450