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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

I believe you can delete the binary file now.

Aside from that and the unnecessary REQUIRES, LGTM.



================
Comment at: llvm/test/tools/llvm-symbolizer/options-from-env.test:1
+REQUIRES: shell
+
----------------
I don't think this `REQUIRES` is needed.


================
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");
 
----------------
phosek wrote:
> jhenderson wrote:
> > 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.
> That's fine with me if we're OK extending the addr2line feature set.
I think we already have extended it, whether we meant to or not!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71668