[PATCH] D34448: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

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?



================
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!
----------------
belleyb wrote:
> 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,
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.


================
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:
----------------
belleyb wrote:
> 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,
Similar comment here.


================
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!
----------------
belleyb wrote:
> 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,
Ditto.


================
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:
----------------
belleyb wrote:
> 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,
Ditto.


https://reviews.llvm.org/D34448