[PATCH] D108985: [objcopy] Enable llvm options to be passed via $LLVM_OPTS

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D108985#2976460 <https://reviews.llvm.org/D108985#2976460>, @sbc100 wrote:

> In D108985#2976307 <https://reviews.llvm.org/D108985#2976307>, @jhenderson wrote:
>
>> Which LLVM options do you and need for what purposes?
>>
>> I've got no real issue with this change in principle, but I think this needs a wider discussion - it doesn't make sense to provide this mechanism in llvm-objcopy on its own, after all, as you've already noted.
>>
>> Some other thoughts:
>>
>> 1. If we do expand this to multiple tools, do we want all tools to share the same variable, or should we have separate variable names? Is it likely that users of multiple tools will want the variable set, but only for some tools?
>
> Yes I think we should add this mechanism to all tool that don't currently call `ParseCommandLineOptions`.  I don't think there are too may such tools but its universally useful to be able to turn on debugging.

It seems like any tool that uses tblgen (which is pretty much all the LLVM binutils, apart from anything else) will need this addition. As the tendency is to move towards tblgen for many tools, this approach may not be ideal (because either we lose functionality, or have to duplicate the code in every tool that is moved over).

>> 2. I'm assuming these options are internal options, and as such, I think "LLVM_OPTS" isn't a great name. Perhaps "LLVM_INTERNAL_OPTS" would be better?
>
> I'm not sure what you mean by "internal" here.  In this case they are debug-only options.   This is a mechanism to passing any `cl::` options in the codebase.  Maybe the term "internal" applies, but I'm not sure what it mean here.

Internal, as-in not really intended for end users (just developers of the tool). If the option is intended for end users, we'd want to document it too.

>> 3. Maybe the options you want to use just want adding as new options in the list of objcopy options?
>
> If I understand correctly that would involve adding extra logic duplicating the handling of `-debug` and `-debug-only` that already exists in `./llvm/lib/Support/Debug.cpp`?   If there was a way to re-using the existing option or forward them somehow it might work?

Right, I was thinking there might be a way to call into the Debug.cpp code when the option is detected.

One issue I thought of with just calling ParseCommandLineOptions blindly is that it'll expose all sorts of options potentially, and those options will be dependent on which files get linked into the end program (due to how cl::opt works). There are options which make no sense to expose, even via that option. For example, what should happen if options like `--version` or `--help` are passed via the environment variable? (Aside: this is also why I want something like "INTERNAL" in the name, because otherwise people might think it's just a general mechanism for passing llvm-objcopy options via the environment variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108985