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

[Benoit Belley via Phabricator via llvm-commits]

belleyb added a comment.

In https://reviews.llvm.org/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. ?



================
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!
----------------
tejohnson wrote:
> 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.
Sure. That makes sense.


https://reviews.llvm.org/D34448