[PATCH] D71668: [llvm-symbolizer] Support reading options from environment

Thu Dec 19 01:34:28 PST 2019

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

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

================
Comment at: llvm/test/tools/llvm-symbolizer/options-from-env.test:9
+
+# RUN: LLVM_SYMBOLIZER_OPTIONS=--debug-file-directory=%p/Inputs llvm-symbolizer --obj=%t 0x20112f | FileCheck --check-prefix=FOUND %s
+
----------------
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.

================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:294
+      argc, argv, IsAddr2Line ? "llvm-addr2line\n" : "llvm-symbolizer\n",
+      /*Errs=*/nullptr, IsAddr2Line ? nullptr : "LLVM_SYMBOLIZER_OPTIONS");

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71668