[PATCH] D81295: lld: initial pass at supporting TBD

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 2 inline comments as done.
compnerd added inline comments.


================
Comment at: lld/test/MachO/iPhoneSimulator.sdk/usr/lib/libSystem.tbd:1
+--- !tapi-tbd-v3
+archs:            [ i386, x86_64 ]
----------------
Ktwu wrote:
> compnerd wrote:
> > Ktwu wrote:
> > > 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.
> > 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.
> > 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?
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.


================
Comment at: lld/test/MachO/stub-link-2.s:1
+# REQUIRES: x86
+
----------------
Ktwu wrote:
> compnerd wrote:
> > Ktwu wrote:
> > > stub-link-2 isn't descriptive. Move this into /test/MachO/invalid and give it a better name like "undefined-stub-symbol"?
> > Its not really an invalid test, its a negative test case.
> "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).
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81295/new/

https://reviews.llvm.org/D81295