[PATCH] D92624: [mac/lld] Add support for the LC_LINKER_OPTION load command in o files

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lld/MachO/Driver.cpp:370-371
+    fatal(Twine(args.getArgString(missingIndex)) + ": missing argument");
+  for (auto *arg : args.filtered(OPT_UNKNOWN))
+    error("ignoring unknown argument: " + arg->getAsString(args));
+
----------------
thakis wrote:
> int3 wrote:
> > is this necessary given that we are going to error out anyway at line 382 below?
> I guess it's not strictly necessary. It matches the flow in parseDirectives() in lld/COFF. The reasoning is probably that this first stage gives you a list of generally valid args, and the 2nd pass below checks which of them are valid in directives / load commands.
I see. How about dropping the "ignoring" and just having "unknown argument:" then? "Ignoring" suggests that we are not erroring out... it looks like `parseDirectives` in the ELF port issues a warning, and `{ELF,MachO}OptTable::parse` just errors with "unknown argument".

As an aside... it's a bit unfortunate that "unknown argument" refers to the CLI flag, while "missing argument" refers to the argument to a flag... but this overload of "argument" seems to happen in many places in the LLVM codebase already so ¯\_(ツ)_/¯


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

https://reviews.llvm.org/D92624