[PATCH] D80582: [lld-macho] Specify the complete set of command-line options for ld64

Thu May 28 19:47:31 PDT 2020

smeenai requested changes to this revision.
smeenai added a comment.
This revision now requires changes to proceed.

> 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?

================
Comment at: lld/MachO/Driver.cpp:80
+  PrintHelp(lld::outs(), (std::string(argv0) + " [options] file...").c_str(),
+            "LLVM Linker", false);
+  lld::outs() << "\n";
----------------
Nit: for boolean arguments, it's conventional to include the name of the argument as a comment, i.e. `/*ShowHidden=*/false` in this case.

================
Comment at: lld/MachO/Driver.cpp:247

-static void handlePlatformVersion(opt::ArgList::iterator &it,
-                                  const opt::ArgList::iterator &end) {
-  // -platform_version takes 3 args, which LLVM's option library doesn't
-  // support directly.  So this explicitly handles that.
-  // FIXME: stash skipped args for later use.
-  for (int i = 0; i < 3; ++i) {
-    ++it;
-    if (it == end || (*it)->getOption().getID() != OPT_INPUT)
-      fatal("usage: -platform_version platform min_version sdk_version");
+template <typename UI, int capacity, int width>
+static bool parsePackedVersion(StringRef str, UI &result) {
----------------
I think something like `components` or `parts` or `numComponents` or `numParts` would be clearer than `capacity`.

================
Comment at: lld/MachO/Driver.cpp:248
+template <typename UI, int capacity, int width>
+static bool parsePackedVersion(StringRef str, UI &result) {
+  result = 0;
----------------
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.)

================
Comment at: lld/MachO/Driver.cpp:250
+  result = 0;
+  SmallVector<StringRef, capacity + 1> parts;
+  str.split(parts, '.', capacity, true);
----------------
Nit: why the `+ 1`? Is it just to account for potentially erroneous input?

================
Comment at: lld/MachO/Driver.cpp:252
+  str.split(parts, '.', capacity, true);
+  auto count = parts.size();
+  if (count < 1 || capacity < count)
----------------
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).

================
Comment at: lld/MachO/Driver.cpp:253
+  auto count = parts.size();
+  if (count < 1 || capacity < count)
+    return true;
----------------
Is it ever possible for the count to be less than 1? Wouldn't that just be an error in `split`?

================
Comment at: lld/MachO/Driver.cpp:293
+  if (parsePackedVersion64(sourceVersionStr, sourceVersion))
+    usageError(arg);
+  // FIXME: add semantics
----------------
The error message output here isn't particularly helpful right now. llvm::Expected should let us address that.

================
Comment at: lld/MachO/Driver.cpp:294
+    usageError(arg);
+  // FIXME: add semantics
+}
----------------
I would imagine the semantics would be storing the result in the Config and then outputting the appropriate load command in the writer?

================
Comment at: lld/MachO/Driver.cpp:298
+static void parsePlatformVersion(const opt::Arg *arg) {
+  static const std::unordered_map<std::string, int> platforms = {
+      {"macos", 1},       {"ios", 2},          {"tvos", 3},    {"watchos", 4},
----------------
Super nit: use a DenseMap of StringRef instead. See https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options.

================
Comment at: lld/MachO/Driver.cpp:318
+      platformNum > 10) {
+    usageError(arg);
+  }
----------------
We should give a specific error message.

================
Comment at: lld/MachO/Driver.cpp:327
+  if (opt.getGroup().getID() == OPT_grp_deprecated) {
+    warn("Option `" + opt.getPrefixedName() + "' is deprecated in ld64:");
+    warn(opt.getHelpText());
----------------
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?

================
Comment at: lld/MachO/Driver.cpp:341
+    warn("Option `" + opt.getPrefixedName() +
+         "' is undocumented for ld64. Should ldd implement it?");
+    break;
----------------
Typo: lld

================
Comment at: lld/MachO/Driver.cpp:393

-  for (opt::ArgList::iterator it = args.begin(), end = args.end(); it != end;
-       ++it) {
-    const opt::Arg *arg = *it;
-    switch (arg->getOption().getID()) {
+  for (auto &arg : args) {
+    const auto &opt = arg->getOption();
----------------
Nit: not that it should matter at all, but can this be `const`?

================
Comment at: lld/MachO/Driver.cpp:401
+      break;
+    default:
+      warnIfUnimplementedOption(opt);
----------------
Nit: have the default at the end.

================
Comment at: lld/MachO/Options.td:5

-def Z: Flag<["-"], "Z">,
-  HelpText<"Do not add standard directories to library search path">;
+def grp_kind : OptionGroup<"kind">, HelpText<"OUTPUT KIND">;

----------------
gkm wrote:
> 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:
>   # hack the help generator to preserve source order--as new behavior? as an option?--or
>   # prefix each `HelpText` with a sequence number or letter so the sorting preserves desired order.
> 
> Preference?
I'm not well versed enough with OptTable to say. @MaskRay, do you have thoughts on this?

================
Comment at: lld/MachO/Options.td:156
+     Group<grp_opts>;
+def platform_version : MultiArg<["-"], "platform_version", 3>,
+     MetaVarName<"<platform> <min_version> <sdk_version>">,
----------------
This is really neat! Today I learned about MultiArg.

================
Comment at: lld/test/MachO/platform-version.test:47
+USAGE: usage: -platform_version <platform> <min_version> <sdk_version>
+USAGE: undefined symbol: _main
+MISSING: -platform_version: missing argument
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80582