[llvm-symbolizer] Support reading options from environment

[phosek]

Summary

llvm-symbolizer is used by sanitizers to symbolize errors discovered by
sanitizer, but there's no way to pass options to llvm-symbolizer since
the tool is invoked directly by the sanitizer runtime. Therefore, we
don't have a way to pass options needed to find debug symbols such as
-dsym-hint or -debug-file-directory. This change enables reading options
from the LLVM_SYMBOLIZER_OPTIONS in addition to command line which can
be used to pass those additional options to llvm-symbolizer invocations
made by sanitizer runtime.

[dblaikie]

@eugenis for sanitizer interaction thoughts

Seems pretty reasonable to me - probably in want of a test case, if possible?

[eugenis]

Looks reasonable. A test case would be great.

[phosek]

Test case added.

[dblaikie]

llvm/test/tools/llvm-symbolizer/options-from-env.test

4–7 ↗

(On Diff #234634)

Can probably skip this test case if there's little chance it could "accidentally" pass (if there's no reason llvm-symbolizer would look in the Inputs directory, it seems unlikely the feature test below would start accidentally passing even if this feature regressed (if llvm-symbolizer started ignoring the environment variable))

[jhenderson]

I like the idea, but the behaviour needs adding to the llvm-symbolizer command guide. You may also want to consider adding it to the section on differences in llvm-addr2line.

(Requesting changes due to this issue - if it's fixed after tonight UK time, I'm okay for somebody else to LGTM and you to commit it then, as Friday is my last day in the office before the New Year).

llvm/test/tools/llvm-symbolizer/options-from-env.test

9 ↗

(On Diff #234634)

I think this test could be simpler and avoid the pre-canned binary by just passing something like --version or --help via the environment variable, and testing they work as expected. Alternatively, something like --addressess as well as some addresses might be a good idea. You don't need to actually have a valid address to show that the address is printed before the '??' stuff.

I'd definitely try moving "0x20122f" (or equivalent) into the environment variable. I know it's not likely to be used that way in common cases, but it seems like it should work.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

294

I'd be inclined to allow llvm-addr2line to read LLVM_ADDR2LINE_OPTIONS. There's no particular reason we should restrict this feature to llvm-symbolizer. We can enhance GNU's feature set safely here, I think.

[phosek]

In D71668#1790886, @jhenderson wrote:

I like the idea, but the behaviour needs adding to the llvm-symbolizer command guide. You may also want to consider adding it to the section on differences in llvm-addr2line.

Done. While looking through command guide, I noticed that lit uses LIT_OPTS for the same purpose, do you have any preference between LLVM_SYMBOLIZER_OPTIONS and LLVM_SYMBOLIZER_OPTS? lit is the only precedence I found in LLVM, other tools seem to be using OPTS as well, e.g. MAKE_OPTS or SPHINXOPTS.

(Requesting changes due to this issue - if it's fixed after tonight UK time, I'm okay for somebody else to LGTM and you to commit it then, as Friday is my last day in the office before the New Year).

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

294

That's fine with me if we're OK extending the addr2line feature set.

[phosek]

I changed the name to LLVM_SYMBOLIZER_OPTS/LLVM_ADDR2LINE_OPTS since the short form seems to be more common among other tools.

[jhenderson]

I believe you can delete the binary file now.

Aside from that and the unnecessary REQUIRES, LGTM.

llvm/test/tools/llvm-symbolizer/options-from-env.test

1 ↗

(On Diff #234766)

I don't think this REQUIRES is needed.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

294

I think we already have extended it, whether we meant to or not!

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

if (IsAddr2Line) {
     ClDemangle.setInitialValue(false);
     ClPrintFunctions.setInitialValue(FunctionNameKind::None);
     ClPrintInlining.setInitialValue(false);
     ClUntagAddresses.setInitialValue(false);
     ClOutputStyle.setInitialValue(DIPrinter::OutputStyle::GNU);
   }
 
   llvm::sys::InitializeCOMRAII COM(llvm::sys::COMThreadingMode::MultiThreaded);
-  cl::ParseCommandLineOptions(argc, argv, IsAddr2Line ? "llvm-addr2line\n"
-                                                      : "llvm-symbolizer\n");
+  cl::ParseCommandLineOptions(
+      argc, argv, IsAddr2Line ? "llvm-addr2line\n" : "llvm-symbolizer\n",
+      /*Errs=*/nullptr, IsAddr2Line ? nullptr : "LLVM_SYMBOLIZER_OPTIONS");
 
   // If both --demangle and --no-demangle are specified then pick the last one.
   if (ClNoDemangle.getPosition() > ClDemangle.getPosition())
     ClDemangle = !ClNoDemangle;
 
   LLVMSymbolizer::Options Opts;
   Opts.PrintFunctions = ClPrintFunctions;
   Opts.UseSymbolTable = ClUseSymbolTable;