bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals

[belleyb]

Summary

• Linker::LinkOnlyNeeded should always import globals with AppendingLinkage.

See: https://bugs.llvm.org/show_bug.cgi?id=33527

[belleyb]

Patch against llvm trunk revision 305867.

[belleyb]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

Linker isn't used by ThinLTO. Include can be removed.

lib/Linker/LinkModules.cpp

334 ↗

(On Diff #103366)

This extra check is the fix (Line 334). Without it, the unit 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,

unittests/Linker/LinkModulesTest.cpp

364 ↗

(On Diff #103366)

Starting from here are added unit tests demonstrating the actual issue and proving the suggested fix is correct.

I was wondering if it would have been more appropriate to write those using llvm-link and FileCheck instead. Let me know if you would prefer that alternate approach instead.

[belleyb]

I have run the entire LLVM & Clang regression test suite on OS X (both Debug & RelWithDebInfo) to ensure that no regressions were introduced.

Running the LLVM regression tests

• Testing: 21053 tests, 8 threads --

Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 155.41s

Expected Passes    : 20177
Expected Failures  : 135
Unsupported Tests  : 741

Running the Clang regression tests

• Testing: 10814 tests, 8 threads --

Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 123.15s

Expected Passes    : 10750
Expected Failures  : 17
Unsupported Tests  : 47

[belleyb]

unittests/Linker/CMakeLists.txt

4 ↗

(On Diff #103366)

For internalizeModule() used by the test.

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

Can you submit this one as a separate NFC change (no patch review needed)? Thanks.

unittests/Linker/LinkModulesTest.cpp

364 ↗

(On Diff #103366)

The unit test is quite long - I typically add tests using FileCheck for this type of issue, if you can demonstrate the issue.

[belleyb]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

I'm not sure how to proceed as I don't have commit access myself. :-(

unittests/Linker/LinkModulesTest.cpp

364 ↗

(On Diff #103366)

Sure, I'll try...

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

Were planning to get commit access for the patch, or if you want I can commit this part for you now?

unittests/Linker/LinkModulesTest.cpp

364 ↗

(On Diff #103366)

Looking at this again, I see it is not one long unit test but multiple tests. However, I still have a preference for a FileCheck lit test, it is just easier to read and see what is being tested. It looks like it should be reproducible using the inputs you have below and llvm-link, piping the output through FileCheck.

402 ↗

(On Diff #103366)

There are a number of references in the unit test to this flag, which looks like it was from an old approach you were considering, but not actually using here. Although I guess this comment is moot if you transition to using lit/FileCheck based tests...

[belleyb]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

I've sent a separate patch for the removal of the include. Would you mind submitting for me ?

http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170619/463344.html

unittests/Linker/LinkModulesTest.cpp

364 ↗

(On Diff #103366)

Agreed. Will do this early next week...

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

35 ↗

(On Diff #103366)

Committed in r306028.

[belleyb]

unittests/Linker/LinkModulesTest.cpp

402 ↗

(On Diff #103366)

Good catch. The llmv-link + FileCheck test will replace all of that...

[belleyb]

Converted the test cases to Lit + llvm-link + FileCheck (instead of unit tests).

(Please ignore Diff 104044, it's a mistake. The correct changes are in Diff 104045.)

[belleyb]

test/Linker/only-needed-compiler-used.ll

8 ↗

(On Diff #104045)

The linker emits the references to the global variables in a different ordering when the --only-needed is used. The explains the use of the DAG FileCheck option below.

test/Linker/only-needed-ctors2.ll

4 ↗

(On Diff #104045)

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,

test/Linker/only-needed-ctors4.ll

4 ↗

(On Diff #104045)

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,

test/Linker/only-needed-dtors2.ll

4 ↗

(On Diff #104045)

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,

test/Linker/only-needed-dtors4.ll

4 ↗

(On Diff #104045)

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,

test/Linker/only-needed-used.ll

8 ↗

(On Diff #104045)

The linker emits the references to the global variables in a different ordering when the --only-needed is used. The explains the use of the DAG FileCheck option below.

[tejohnson]

Thanks and very thorough testing! I'm not sure all these combinations are needed though, I have some suggestions on paring it down. Once that is done, LGTM. Do you want me to commit for you, or did you want to get commit access?

test/Linker/only-needed-ctors2.ll

4 ↗

(On Diff #104045)

I'm not sure this test is valid/useful - linking two global definitions of func1 seems like a user error in any case. Suggest removing this one along with only-needed-ctors.src.b unless there is something particularly tricky about this combination that you wanted to check for. You have pretty thorough testing already. If it is needed, then I suggest making func1 internal to start with (or at least one of the func1), and then removing only-needed-ctors1.ll.

test/Linker/only-needed-ctors4.ll

4 ↗

(On Diff #104045)

Similar comment here.

test/Linker/only-needed-dtors2.ll

4 ↗

(On Diff #104045)

Ditto.

test/Linker/only-needed-dtors4.ll

4 ↗

(On Diff #104045)

Ditto.

[belleyb]

In D34448#792170, @tejohnson wrote:

Thanks and very thorough testing! I'm not sure all these combinations are needed though, I have some suggestions on paring it down. Once that is done, LGTM. Do you want me to commit for you, or did you want to get commit access?

One way of the other, I don't mind. Would it be easier for everyone if I had commit access ? Can you grant that to me ? Or, do we have to involve Chris L. ?

test/Linker/only-needed-ctors2.ll

4 ↗

(On Diff #104045)

Sure. That makes sense.

[belleyb]

Re-organized and simplified test cases to address code review comments.

[belleyb]

test/Linker/Inputs/only-needed-ctors.ll

14 ↗

(On Diff #104320)

I have added an unused function, just to make sure that the -only-needed switch produces a different result.

[tejohnson]

LGTM

In D34448#792355, @belleyb wrote:

In D34448#792170, @tejohnson wrote:

Thanks and very thorough testing! I'm not sure all these combinations are needed though, I have some suggestions on paring it down. Once that is done, LGTM. Do you want me to commit for you, or did you want to get commit access?

One way of the other, I don't mind. Would it be easier for everyone if I had commit access ? Can you grant that to me ? Or, do we have to involve Chris L. ?

I don't mind committing for you. If you plan to make more contributions, you will probably find it more convenient to have commit access. It does involve emailing Chris, but that is usually pretty fast I believe. Here are the instructions if you decide to do this in the future:
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
But let me know if you want I can go ahead and submit this for you.

[belleyb]

Just to let you know that I've requested commit access.

llvm/trunk/lib/Linker/LinkModules.cpp

assert(Dest.hasExternalLinkage() && Src.hasExternalLinkage() &&
          "Unexpected linkage type!");
   return emitError("Linking globals named '" + Src.getName() +
                    "': symbol multiply defined!");
 }
 
 bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
   GlobalValue *DGV = getLinkedToGlobal(&GV);
 
-  if (shouldLinkOnlyNeeded() && !(DGV && DGV->isDeclaration()))
+  if (shouldLinkOnlyNeeded()) {
+    // Always import variables with appending linkage.
+    if (!GV.hasAppendingLinkage()) {
+      // Don't import globals unless they are referenced by the destination
+      // module.
+      if (!DGV)
+        return false;
+      // Don't import globals that are already defined in the destination module
+      if (!DGV->isDeclaration())
         return false;
+    }
+  }
 
   if (DGV && !GV.hasLocalLinkage() && !GV.hasAppendingLinkage()) {
     auto *DGVar = dyn_cast<GlobalVariable>(DGV);
     auto *SGVar = dyn_cast<GlobalVariable>(&GV);
     if (DGVar && SGVar) {
       if (DGVar->isDeclaration() && SGVar->isDeclaration() &&
           (!DGVar->isConstant() || !SGVar->isConstant())) {
         DGVar->setConstant(false);

llvm/trunk/test/Linker/Inputs/only-needed-compiler-used.ll

+@used1 = global i8 4
+@used2 = global i32 123
+
[e-mail] = appending global [2 x i8*] [
+   i8* @used1,
+   i8* bitcast (i32* @used2 to i8*)
+], section "llvm.metadata"

llvm/trunk/test/Linker/Inputs/only-needed-ctors.ll

+define internal void @ctor1() {
+  call void @func1()
+  ret void
+}
+
+define internal void @ctor2() {
+  ret void
+}
+
+define void @func1() {
+  ret void
+}
+
+define void @unused() {
+  ret void
+}
+
[e-mail]_ctors = appending global[2 x{i32, void() *, i8 * }] [
+    {i32, void() *, i8 * } { i32 2, void() *@ctor1, i8 *null},
+    {i32, void() *, i8 * } { i32 7, void() *@ctor2, i8 *null}]

llvm/trunk/test/Linker/Inputs/only-needed-dtors.ll

+define internal void @dtor1() {
+  call void @func1()
+  ret void
+}
+
+define internal void @dtor2() {
+  ret void
+}
+
+define void @func1() {
+  ret void
+}
+
+define void @unused() {
+  ret void
+}
+
[e-mail]_dtors = appending global[2 x{i32, void() *, i8 * }] [
+    {i32, void() *, i8 * } { i32 2, void() *@dtor1, i8 *null},
+    {i32, void() *, i8 * } { i32 7, void() *@dtor2, i8 *null}]

llvm/trunk/test/Linker/Inputs/only-needed-used.ll

+@used1 = global i8 4
+@used2 = global i32 123
+
[e-mail] = appending global [2 x i8*] [
+   i8* @used1,
+   i8* bitcast (i32* @used2 to i8*)
+], section "llvm.metadata"

llvm/trunk/test/Linker/only-needed-compiler-used.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-compiler-used.ll | FileCheck %s --check-prefix=CHECK --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-compiler-used.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-compiler-used.ll | FileCheck %s --check-prefix=CHECK --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-compiler-used.ll | FileCheck %s --check-prefix=CHECK --check-prefix=INTERNALIZE
+
+; Empty destination module!
+
+
+; CHECK-DAG:          @llvm.compiler.used = appending global [2 x i8*] [i8* @used1, i8* bitcast (i32* @used2 to i8*)], section "llvm.metadata"
+; NO-INTERNALIZE-DAG: @used1 = global i8 4
+; INTERNALIZE-DAG:    @used1 = internal global i8 4
+; NO-INTERNALIZE-DAG: @used2 = global i32 123
+; INTERNALIZE-DAG:    @used2 = internal global i32 123

llvm/trunk/test/Linker/only-needed-ctors1.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK-ALL    --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK_ALL    --check-prefix=INTERNALIZE
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=INTERNALIZE
+
+; Empty destination module!
+
+
+; CHECK:           @llvm.global_ctors = appending global [2 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 2, void ()* @ctor1, i8* null }, { i32, void ()*, i8* } { i32 7, void ()* @ctor2, i8* null }]
+; CHECK:           define internal void @ctor1()
+; CHECK:           define internal void @ctor2()
+; NO-INTERNALIZE:  define void @func1()
+; INTERNALIZE:     define internal void @func1()
+; LINK-ALL:        define {{(internal )?}}void @unused()
+; ONLY-NEEDED-NOT: void @unused()

llvm/trunk/test/Linker/only-needed-ctors2.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK-ALL    --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK_ALL    --check-prefix=INTERNALIZE
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-ctors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=INTERNALIZE
+
+; Destination module:
+
+define void @foo() {
+  ret void
+}
+
+define internal void @ctor1() {
+  ret void
+}
+
[e-mail]_ctors = appending global[1 x{i32, void() *, i8 * }] [
+    {i32, void() *, i8 * } { i32 4, void() *@ctor1, i8 *null}]
+
+
+; CHECK:           @llvm.global_ctors = appending global [3 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 4, void ()* @ctor1, i8* null }, { i32, void ()*, i8* } { i32 2, void ()* @ctor1.2, i8* null }, { i32, void ()*, i8* } { i32 7, void ()* @ctor2, i8* null }]
+; CHECK:           define internal void @ctor1()
+; CHECK:           define void @foo()
+; CHECK:           define internal void @ctor1.{{[0-9]+}}()
+; CHECK:           define internal void @ctor2()
+; NO-INTERNALIZE:  define void @func1()
+; INTERNALIZE:     define internal void @func1()
+; LINK-ALL:        define {{(internal )?}}void @unused()
+; ONLY-NEEDED-NOT: void @unused()

llvm/trunk/test/Linker/only-needed-dtors1.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK-ALL    --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK_ALL    --check-prefix=INTERNALIZE
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=INTERNALIZE
+
+; Empty destination module!
+
+
+; CHECK:           @llvm.global_dtors = appending global [2 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 2, void ()* @dtor1, i8* null }, { i32, void ()*, i8* } { i32 7, void ()* @dtor2, i8* null }]
+; CHECK:           define internal void @dtor1()
+; CHECK:           define internal void @dtor2()
+; NO-INTERNALIZE:  define void @func1()
+; INTERNALIZE:     define internal void @func1()
+; LINK-ALL:        define {{(internal )?}}void @unused()
+; ONLY-NEEDED-NOT: void @unused()

llvm/trunk/test/Linker/only-needed-dtors2.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK-ALL    --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=LINK_ALL    --check-prefix=INTERNALIZE
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=NO-INTERNALIZE
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-dtors.ll | FileCheck %s --check-prefix=CHECK --check-prefix=ONLY-NEEDED --check-prefix=INTERNALIZE
+
+; Destination module:
+
+define void @foo() {
+  ret void
+}
+
+define internal void @dtor1() {
+  ret void
+}
+
[e-mail]_dtors = appending global[1 x{i32, void() *, i8 * }] [
+    {i32, void() *, i8 * } { i32 4, void() *@dtor1, i8 *null}]
+
+
+; CHECK:           @llvm.global_dtors = appending global [3 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 4, void ()* @dtor1, i8* null }, { i32, void ()*, i8* } { i32 2, void ()* @dtor1.2, i8* null }, { i32, void ()*, i8* } { i32 7, void ()* @dtor2, i8* null }]
+; CHECK:           define internal void @dtor1()
+; CHECK:           define void @foo()
+; CHECK:           define internal void @dtor1.{{[0-9]+}}()
+; CHECK:           define internal void @dtor2()
+; NO-INTERNALIZE:  define void @func1()
+; INTERNALIZE:     define internal void @func1()
+; LINK-ALL:        define {{(internal )?}}void @unused()
+; ONLY-NEEDED-NOT: void @unused()

llvm/trunk/test/Linker/only-needed-used.ll

+; RUN: llvm-link -S                           %s %p/Inputs/only-needed-used.ll | FileCheck %s
+; RUN: llvm-link -S              -internalize %s %p/Inputs/only-needed-used.ll | FileCheck %s
+; RUN: llvm-link -S -only-needed              %s %p/Inputs/only-needed-used.ll | FileCheck %s
+; RUN: llvm-link -S -only-needed -internalize %s %p/Inputs/only-needed-used.ll | FileCheck %s
+
+; Empty destination module!
+
+
+; CHECK-DAG:          @llvm.used = appending global [2 x i8*] [i8* @used1, i8* bitcast (i32* @used2 to i8*)], section "llvm.metadata"
+; CHECK-DAG: @used1 = global i8 4
+; CHECK-DAG: @used2 = global i32 123