This is an archive of the discontinued LLVM Phabricator instance.

lld: initial pass at supporting TBD
ClosedPublic

Authored by compnerd on Jun 5 2020, 12:15 PM.

Details

Reviewers
smeenai
int3
Ktwu
Summary

Add support to lld to use Text Based API stubs for linking. This is
support is incomplete not filtering out platforms. It also does not
account for architecture specific API handling and potentially does not
correctly handle trees of re-exports with inlined libraries being
treated as direct children of the top level library.

Diff Detail

Event Timeline

compnerd created this revision.Jun 5 2020, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 12:15 PM
Ktwu added a comment.Jun 5 2020, 2:26 PM

This is largely incomplete, not filtering out platforms and architectures.

This is a must-have for an intro diff imo. Unfortunately filtering things down isn't as simple as what @int3 implemented for true dylibs since architecture and platform information is per symbol, not a chunk of the file...

compnerd updated this revision to Diff 268967.Jun 5 2020, 5:14 PM

Update test to validate the binding

compnerd updated this revision to Diff 268984.Jun 5 2020, 7:00 PM

Fix memory corruption in the binding name. We were holding a reference to a StringRef beyond the lifetime of the buffer.

Add a failing demonstrating the failure to filter out the architecture specific symbols.

compnerd updated this revision to Diff 268989.Jun 5 2020, 8:05 PM

Add support for filtering architectures. Fix the negative test case.

Ktwu added a comment.Jun 8 2020, 10:19 AM

This is largely incomplete, not filtering out platforms and architectures.

Not true anymore :D

potentially does not correct handle trees of re-exports with inlined

libraries being treated as direct children of the top level library.

Add a TODO?

lld/test/MachO/iPhoneSimulator.sdk/usr/lib/libSystem.tbd
1

It's nice to know where these files came from, but there's not a need to create a full copy that even preserves the path, right? If anything, you should be able to combine the two stub files into one manually and use them both for your test.

For example, you don't use crashreporeter_info, so get rid of it.

lld/test/MachO/stub-link-2.s
1

stub-link-2 isn't descriptive. Move this into /test/MachO/invalid and give it a better name like "undefined-stub-symbol"?

compnerd marked 2 inline comments as done.Jun 8 2020, 11:09 AM
compnerd added inline comments.
lld/test/MachO/iPhoneSimulator.sdk/usr/lib/libSystem.tbd
1

Keeping them separate is important IMO, and far more useful. This is going to grow over time, and get used for future tests as the functionality improves. The paths here are not indicative of where they originate from - the UUIDs are pretty obviously sequential - as these are fabricated. The versions happen to be cause I had a tab open to some revision of opensource.apple.com.

The single symbol there is there because its useful for testing symbol origins from the parent umbrella vs re-exported dylibs.

lld/test/MachO/stub-link-2.s
1

Its not really an invalid test, its a negative test case.

compnerd updated this revision to Diff 269295.Jun 8 2020, 11:16 AM
compnerd edited the summary of this revision. (Show Details)

Update commit message, add some TODOs as suggested.

Ktwu added inline comments.Jun 8 2020, 11:24 AM
lld/test/MachO/iPhoneSimulator.sdk/usr/lib/libSystem.tbd
1

This is going to grow over time, and get used for future tests as the functionality improves.

Hmmm, I was under the impression that it's better to be intentional in testing for supported features (don't add red herrings in tests that could give a reader a false impression of which scenarios are tested).

The single symbol there is there because its useful for testing symbol origins from the parent umbrella vs re-exported dylibs.

But this isn't actively tested :(

The versions happen to be cause I had a tab open to some revision of opensource.apple.com

Ok, since the paths aren't really important, can you flatten them and stick them into lld/test/Macho/Inputs?

lld/test/MachO/stub-link-2.s
1

"invalid" is where negative / error-based tests live for lld (or ought to live, there are probably a few exceptions from before thinking such a place was a good idea).

compnerd marked 2 inline comments as done.Jun 8 2020, 12:25 PM
compnerd added inline comments.
lld/test/MachO/iPhoneSimulator.sdk/usr/lib/libSystem.tbd
1

I agree about being intentional about testing. However, reconstructing this seems unnecessary. This is going to end up getting used in short order. The TBD support here is very rudimentary and needs to expand. This is related to one of the TODOs that is mentioned. I don't think that the extra symbol is that big of a deal and is something that will make it easier for expanding the test coverage.

Sure, moving the SDKs into the Inputs is fine, I'll move them. However, keeping the SDKs as SDKs is important IMO as they make it easier to understand what is being tested. It makes it easier to understand the test cases when you are looking at the invocation.

lld/test/MachO/stub-link-2.s
1

Hmm, okay, I can move the test there. I think that we should remove the directory - this is going against the rest of the project's practices and creates unnecessary friction.

compnerd updated this revision to Diff 269320.Jun 8 2020, 12:42 PM

shuffle files around as recommended by @Ktwu

Ktwu accepted this revision.Jun 8 2020, 12:54 PM

Thanks!

This revision is now accepted and ready to land.Jun 8 2020, 12:54 PM