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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/test/tools/llvm-objcopy/llvm-options.test:1
+RUN: llvm-objcopy --version 2>&1 | FileCheck %s
+RUN: LLVM_OPTS=-debug llvm-objcopy --version 2>&1 | FileCheck --check-prefix=DEBUG %s
----------------
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."


================
Comment at: llvm/test/tools/llvm-objcopy/llvm-options.test:2
+RUN: llvm-objcopy --version 2>&1 | FileCheck %s
+RUN: LLVM_OPTS=-debug llvm-objcopy --version 2>&1 | FileCheck --check-prefix=DEBUG %s
+
----------------
You need the env command to set environment variables for test commands. The current approach fails on Windows.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:95
     return parseBitcodeStripOptions(Args);
-  else if (Is("strip"))
+  } else if (Is("strip")) {
+    LLVM_DEBUG(llvm::dbgs() << "running as strip\n");
----------------
This clang-tidy issue predates your change, but you might as well fix it whilst you're modifying the lines.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:429
+  // 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");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108985