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

[sbc100]

Summary

I was recently debugging an issue with llvm-objcopy and noticed that
there was no way to set -debug or -debug-only=xxx options.

Even though llvm-objcopy does it own custom arugment parsing I think it
is still useful to be able set common/shared llvm options like this.

[sbc100]

Still trying to figure out a good way to test this. Also, perhaps this should also be included in any other tools that don't already use ParseCommandLineOptions?

[sbc100]

add test

[jhenderson]

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?
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?
3. Maybe the options you want to use just want adding as new options in the list of objcopy options?

llvm/test/tools/llvm-objcopy/llvm-options.test

1 ↗

(On Diff #369677)

Perhaps worth a simple comment explaining what this test is for. Something like "Show that LLVM_OPTS can be used to pass internal LLVM options."

2 ↗

(On Diff #369677)

You need the env command to set environment variables for test commands. The current approach fails on Windows.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

91

This clang-tidy issue predates your change, but you might as well fix it whilst you're modifying the lines.

421

I'd change "common flags" to "internal LLVM flags", because the options aren't really intended for general users of the tool, if I'm not mistaken.

[MaskRay]

-debug or -debug-only=xxx is for IR optimization and code generation debugging.
I am not sure such options are generally useful to llvm-objcopy.
Non-assert builds even make such options no-op, so I am concerned that LLVM_OPTS will be used to do such debugging.

I agree that this needs a wide discussion.

[sbc100]

In D108985#2976316, @MaskRay wrote:

-debug or -debug-only=xxx is for IR optimization and code generation debugging.

I don't see anything IR specific about the -debug or -debug-only options. Anyone who uses the include/llvm/Support/Debug.h facilities relies on being able to set these options. I use them a lot when debugging wasm-ld for example, and in this case I was specifically interesting in the debugging libObject (where I use these macros: https://github.com/llvm/llvm-project/blob/fb321c2ea274c1bd1e1b294f2c090f1c0d9a97fe/llvm/lib/Object/WasmObjectFile.cpp#L35) usage in llvm-objcopy

I am not sure such options are generally useful to llvm-objcopy.
Non-assert builds even make such options no-op, so I am concerned that LLVM_OPTS will be used to do such debugging.

True these option only make sense in debug builds.. but that is true for all our tools such as clang and lld too. They both provide ways of accepting these options. As do any of the tools under llvm/tools that already call ParseCommandLineOptions. I think it would be useful to add the ability to accept these args in all tools, otherwise there is no way to enable the debugging code which is built into them.

I agree that this needs a wide discussion.

[sbc100]

In 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.

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.

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?

[sbc100]

• feedback

[jhenderson]

In D108985#2976460, @sbc100 wrote:

In 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.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

auto Is = [=](StringRef Tool) {
     // llvm-install-name-tool -> install-name-tool
     auto I = Stem.rfind_insensitive(Tool);
     return I != StringRef::npos &&
            (I + Tool.size() == Stem.size() || !isAlnum(Stem[I + Tool.size()]));
   };
 
   if (Is("bitcode-strip") || Is("bitcode_strip"))
     return parseBitcodeStripOptions(Args);
   else if (Is("strip"))
     return parseStripOptions(Args, reportWarning);
   else if (Is("install-name-tool") || Is("install_name_tool"))
     return parseInstallNameToolOptions(Args);
   else
     return parseObjcopyOptions(Args, reportWarning);
 }
 
 // For regular archives this function simply calls llvm::writeArchive,
int main(int argc, char **argv) {
   SmallVector<const char *, 20> NewArgv(argv, argv + argc);
   BumpPtrAllocator A;
   StringSaver Saver(A);
   cl::ExpandResponseFiles(Saver,
                           Triple(sys::getProcessTriple()).isOSWindows()
                               ? cl::TokenizeWindowsCommandLine
                               : cl::TokenizeGNUCommandLine,
                           NewArgv);
+  // Allow llvm options to be passed via $LLVM_OPTS.  This is useful for
+  // enabling common flags such as -debug.
+  cl::ParseCommandLineOptions(1, argv, "llvm-objcopy", nullptr, "LLVM_OPTS");
 
   auto Args = makeArrayRef(NewArgv).drop_front();
   Expected<DriverConfig> DriverConfig = getDriverConfig(Args);
 
   if (!DriverConfig) {
     logAllUnhandledErrors(DriverConfig.takeError(),
                           WithColor::error(errs(), ToolName));
     return 1;