This is an archive of the discontinued LLVM Phabricator instance.

bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals
ClosedPublic

Authored by belleyb on Jun 21 2017, 6:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

belleyb created this revision.Jun 21 2017, 6:28 AM

Patch against llvm trunk revision 305867.

belleyb added inline comments.Jun 21 2017, 6:44 AM
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.

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 added inline comments.Jun 21 2017, 7:03 AM
unittests/Linker/CMakeLists.txt
4 ↗(On Diff #103366)

For internalizeModule() used by the test.

belleyb edited the summary of this revision. (Show Details)Jun 21 2017, 7:36 AM
tejohnson added inline comments.Jun 22 2017, 7:31 AM
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 marked 2 inline comments as done.Jun 22 2017, 7:42 AM
belleyb added inline comments.
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 added inline comments.Jun 22 2017, 8:00 AM
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 marked 7 inline comments as done.Jun 22 2017, 8:17 AM
belleyb added inline comments.
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...

belleyb marked 3 inline comments as done.Jun 22 2017, 8:39 AM
tejohnson added inline comments.Jun 22 2017, 9:21 AM
lib/LTO/ThinLTOCodeGenerator.cpp
35 ↗(On Diff #103366)

Committed in r306028.

belleyb edited the summary of this revision. (Show Details)Jun 26 2017, 5:07 AM
belleyb marked an inline comment as done.Jun 26 2017, 11:49 AM
belleyb added inline comments.
unittests/Linker/LinkModulesTest.cpp
402 ↗(On Diff #103366)

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

belleyb updated this revision to Diff 104044.EditedJun 26 2017, 5:04 PM
belleyb marked an inline comment as done.

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 updated this revision to Diff 104045.Jun 26 2017, 5:07 PM
belleyb added inline comments.Jun 26 2017, 5:17 PM
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.

belleyb marked 6 inline comments as done.Jun 26 2017, 5:22 PM
tejohnson accepted this revision.Jun 27 2017, 8:09 AM

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.

This revision is now accepted and ready to land.Jun 27 2017, 8:09 AM

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 updated this revision to Diff 104320.Jun 27 2017, 5:08 PM

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

belleyb marked 3 inline comments as done.Jun 27 2017, 5:11 PM
belleyb added inline comments.
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.

belleyb marked an inline comment as done.Jun 27 2017, 5:11 PM
tejohnson accepted this revision.Jun 28 2017, 7:06 AM

LGTM

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.

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

This revision was automatically updated to reflect the committed changes.