This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Support reading options from environment
ClosedPublic

Authored by phosek on Dec 18 2019, 10:24 AM.

Details

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.

Diff Detail

Event Timeline

phosek created this revision.Dec 18 2019, 10:24 AM
Herald added a project: Restricted Project. · View Herald Transcript

@eugenis for sanitizer interaction thoughts

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

Looks reasonable. A test case would be great.

phosek updated this revision to Diff 234634.Dec 18 2019, 4:14 PM

Test case added.

dblaikie accepted this revision.Dec 18 2019, 4:23 PM
dblaikie added inline comments.
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))

This revision is now accepted and ready to land.Dec 18 2019, 4:23 PM
jhenderson requested changes to this revision.Dec 19 2019, 1:32 AM

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.

This revision now requires changes to proceed.Dec 19 2019, 1:32 AM
phosek updated this revision to Diff 234765.Dec 19 2019, 12:08 PM
phosek marked 4 inline comments as done.

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 updated this revision to Diff 234766.Dec 19 2019, 12:12 PM

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

jhenderson accepted this revision.Dec 20 2019, 12:27 AM

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!

This revision is now accepted and ready to land.Dec 20 2019, 12:27 AM
phosek updated this revision to Diff 234951.Dec 20 2019, 12:47 PM
This revision was automatically updated to reflect the committed changes.