This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Specify the complete set of command-line options for ld64
ClosedPublic

Authored by gkm on May 26 2020, 12:51 PM.

Details

Summary

This is a complete Options.td compiled from ld(1) dated 2018-03-07 and cross checked with ld64 source code version 512.4 dated 2018-03-18.

This is the first in a series of diffs for argument handling. Follow-ups will include switch cases for all the new instances of OPT_foo, and parsing/validation of arguments attached to options, e.g., more code akin to OPT_platform_version and associated parsePlatformVersion().

Test Plan: check-lld

Diff Detail

Unit TestsFailed

Event Timeline

gkm created this revision.May 26 2020, 12:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 12:51 PM
gkm edited the summary of this revision. (Show Details)May 26 2020, 1:07 PM
gkm added inline comments.May 26 2020, 1:16 PM
lld/MachO/Options.td
5

I notice that the --help output sorts option groups alphabetically by the group's HelpText. That's not so great. I would rather see the groups presented in the order listed here in Options.td.

Two remedies:

  1. hack the help generator to preserve source order--as new behavior? as an option?--or
  2. prefix each HelpText with a sequence number or letter so the sorting preserves desired order.

Preference?

Will review this in detail later, but quick question: how is this handling erroring for options which are in the .td but aren't implemented yet?

gkm updated this revision to Diff 266340.May 26 2020, 2:59 PM

Issue a variety of warnings about unhandled options

gkm updated this revision to Diff 266384.May 26 2020, 5:26 PM

add usageError() to report any option's expected args

gkm updated this revision to Diff 266411.May 26 2020, 9:02 PM
  • check that an option group is valid before getting its ID
  • always warn about deprecated options, even when handled
gkm updated this revision to Diff 266426.May 26 2020, 10:21 PM
  • fix clang-tidy warnings, mostly pertaining to pushing camelCase over snake_case
gkm updated this revision to Diff 266427.May 26 2020, 10:25 PM
  • fix another clang-tidy warning for camelCase over snake_case
gkm updated this revision to Diff 266444.EditedMay 27 2020, 12:47 AM
  • add current-version.test
  • use StringRef::split() instead of llvm::SplitString() for better handling of empty fields between '.' separators
  • reject empty and excess version# components
  • fix calculation of bit width for the first version component
gkm retitled this revision from Specify the complete set of command-line options for ld64 to [lld] Specify the complete set of command-line options for ld64.May 27 2020, 12:20 PM
gkm retitled this revision from [lld] Specify the complete set of command-line options for ld64 to [lld-macho] Specify the complete set of command-line options for ld64.
gkm updated this revision to Diff 266679.May 27 2020, 2:49 PM
  • s/platform/platformNum/ for clarity
gkm updated this revision to Diff 267101.May 28 2020, 6:04 PM

rebase

smeenai requested changes to this revision.May 28 2020, 7:24 PM

This is a complete Options.td compiled from ld(1) dated 2018-03-07 and cross checked with ld64 source code version 512.4 dated 2018-03-18.

Out of curiosity, where are you getting these dates from? I'd imagine 512.4 to be much more recent than that.

Follow-ups will include switch cases for all the new instances of OPT_foo

Could you elaborate on what you mean by this?

Gathering all the options and producing the TableGen is a pretty huge effort. Thanks for doing this!

Can you add a test for some of the deprecated, obsolete, unimplemented, etc. options to ensure we give the right messages for them?

lld/MachO/Driver.cpp
80

Nit: for boolean arguments, it's conventional to include the name of the argument as a comment, i.e. /*ShowHidden=*/false in this case.

247–353

I think something like components or parts or numComponents or numParts would be clearer than capacity.

248

This should return an llvm::Expected instead of a boolean, which would also allow you to have different error messages for the various parse failures.

My understanding is that the packing is needed because that's how the corresponding load commands are written out. In that case, I'd argue that there's multiple concerns going on in this function. Ideally this would only parse the version out into a tuple, and then the packing would be a separate function in the writer that's called when you're writing out the load command. The other advantage of that would be that if we need to make decisions based on these values, a tuple is a lot easier to process than a packed value. (E.g. ld64 emits either an LC_BUILD_VERSION or an LC_VERSION_MIN_MACOSX depending on your minimum version.)

250

Nit: why the + 1? Is it just to account for potentially erroneous input?

252

LLD isn't a big fan of auto. I'd just use an explicit size_t (or if you get rid of the < 1 check below, you can inline the call to size and avoid the variable entirely :D).

253

Is it ever possible for the count to be less than 1? Wouldn't that just be an error in split?

293

The error message output here isn't particularly helpful right now. llvm::Expected should let us address that.

294

I would imagine the semantics would be storing the result in the Config and then outputting the appropriate load command in the writer?

298

Super nit: use a DenseMap of StringRef instead. See https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options.

318

We should give a specific error message.

327

I'm torn about referencing ld64 in these messages. On the one hand, it is the motivation for our behavior. On the other hand, all our users might not know what ld64 is (or at least know it by that name), or they might be confused why LLD is giving error messages that reference ld64.

I'm leaning towards just making these say "is deprecated" instead of "is deprecated in ld64". What do you think?

341

Typo: lld

393–403

Nit: not that it should matter at all, but can this be const?

401

Nit: have the default at the end.

lld/MachO/Options.td
5

I'm not well versed enough with OptTable to say. @MaskRay, do you have thoughts on this?

156

This is really neat! Today I learned about MultiArg.

lld/test/MachO/platform-version.test
47

I wouldn't add this check, since it's not the main point of the test, and it won't be produced if we bail out early on input argument errors.

This revision now requires changes to proceed.May 28 2020, 7:24 PM
thakis added a subscriber: thakis.Jun 2 2020, 6:02 AM

Taking a step back: Maybe it makes sense to only add flags that people ask for? For example, I'm guessing nobody needs Xcode 3 and earlier flags any more. That's also the strategy that the elf and coff ports went for, and I feel it's worked fairly well there.

Taking a step back: Maybe it makes sense to only add flags that people ask for? For example, I'm guessing nobody needs Xcode 3 and earlier flags any more. That's also the strategy that the elf and coff ports went for, and I feel it's worked fairly well there.

@gkm can chime in here as well, but my understanding was that we were only planning to add the actual support for flags once there was a use case. There's some nice things we get from having the entirety of ld64's interface expressed in our tablegen though:

  • It gives a crude sense of our completeness, in terms of how many ld64 options we support.
  • We can emit nicer diagnostics that distinguish between options we don't support yet, options that are deprecated and we're therefore unlikely to ever support, and options that are just completely invalid.
  • It sets up the scaffolding to easily add support for more options later (since they'll already be in the tablegen).

There's some downsides though:

  • Our --help output might become overwhelming, especially when most of it is currently unsupported options. We could hide unsupported options from the help output, or else group the help output in a way that makes the supported/unsupported status clear.
  • For some options, we're likely never going to support them (e.g. the Xcode 3 and earlier flags, like you said). There's also some options that we're realistically not planning to support any time soon (although contributions are most welcome, of course). The only purpose the TableGen entries for those are serving is to get nicer diagnostics.
  • Options.td gets a bit cluttered, though we can e.g. organize it along supported/unsupported categories to make it easier to read.

Does that make sense to you?

aaron.ballman added inline comments.Jun 9 2020, 6:30 PM
lld/MachO/Options.td
524

How about: Lists which dylibs and frameworks by <name> that are allowed to link to this dylib or something along those lines?

gkm updated this revision to Diff 270034.Jun 10 2020, 10:13 PM
gkm marked 12 inline comments as done.
  • Unimplemented options get Flags<[HelpHidden]> in lld/MachO/Options.td
  • Retain only core changes to Options.td + diagnostic infra - future diffs will implement particular options
gkm marked 9 inline comments as done.Jun 10 2020, 10:28 PM
gkm added a subscriber: compnerd.
gkm added inline comments.
lld/MachO/Driver.cpp
248

I am deferring to @compnerd's D81413 for -platform-version improvements , so am removing my changes from this diff.

250

Yes: if there are excess components, this will allow split to detect them.

294

Yes

327

How about saying "... is deprecated by Apple"?

lld/test/MachO/platform-version.test
47

The complaint about undefined _main happens after the arg-parsing phase, by expecting it, we are asserting that no unexpected diagnostic appears during arg parsing.
What do you recommend as an alternative method?

smeenai accepted this revision.Jun 11 2020, 11:35 PM

HelpHidden assuages my primary user-facing concern about the help text :)

There's a bunch of options we're in all probability going to *never* support, e.g. the Obj-C GC ones, since Obj-C GC is completely unsupported now. I don't know if it's better to remove them entirely or have a different category of "options that LLD will never support", but we can figure that out later.

lld/MachO/Driver.cpp
250

Makes sense, although to be clear, the template argument for a SmallVector is the number of elements that can be inlined. The SmallVector can still grow beyond that; its storage will just need to be heap allocated.

327

That's technically true, but I also feel a bit uncomfortable speaking on behalf of another company :/

369

Nit: indicate the name of the boolean parameter as a comment, as in /*helpHidden=*/true

372

Same here.

lld/MachO/Options.td
7

You should double check this date ... I'm pretty sure 512.4 is more recent than that.

lld/test/MachO/platform-version.test
47

I misunderstood what you were testing here. This makes sense.

This revision is now accepted and ready to land.Jun 11 2020, 11:35 PM
gkm updated this revision to Diff 270504.Jun 12 2020, 1:10 PM
gkm marked 9 inline comments as done.
  • Incorporate review feedback. Approved & ready to land!
gkm added inline comments.Jun 12 2020, 1:20 PM
lld/MachO/Options.td
7

Perhaps the release date was later, but the date on the git tag 512.4 is as stated.

From the ld64 source tree at https://github.com/apple-opensource/ld64:

$ git show 512.4 |head
tag 512.4
Tagger: Apple Open Source <apple.opensource@gmail.com>
Date:   Wed Mar 18 21:18:04 2020 +0000

512.4
-----BEGIN PGP SIGNATURE-----
. . .
tschuett added a subscriber: tschuett.EditedJun 12 2020, 1:59 PM

https://opensource.apple.com/source/ld64/ld64-530/doc/man/man1/ld.1.auto.html
https://opensource.apple.com/source/ld64/ld64-530/

The latest open source version is at 530 and the date of the man page is also March 7, 2018.
My ld64 is version 556.6 and the man page did not change since March 7, 2018.

This revision was automatically updated to reflect the committed changes.