This is an archive of the discontinued LLVM Phabricator instance.

Let ubsan search UBSAN_SYMBOLIZER_PATH for llvm-symbolizer
ClosedPublic

Authored by thakis on Dec 2 2016, 4:05 PM.

Details

Reviewers
samsonov
eugenis
Summary

There are ASAN_SYMBOLIZER_PATH and MSAN_SYMBOLIZER_PATH, I found it surprising that UBSAN_SYMBOLIZER_PATH didn't exist.

Diff Detail

Event Timeline

thakis updated this revision to Diff 80150.Dec 2 2016, 4:05 PM
thakis retitled this revision from to Let ubsan search UBSAN_SYMBOLIZER_PATH for llvm-symbolizer.
thakis updated this object.
thakis added reviewers: eugenis, samsonov.
thakis added a subscriber: llvm-commits.
eugenis edited edge metadata.Dec 2 2016, 4:09 PM

how about adding LLVM_SYMBOLIZER_PATH that works in all sanitizers?

thakis added a comment.Dec 2 2016, 4:17 PM

Works for me too. After reading the code, I think setting (ASAN|UBSAN|MSAN)_OPTIONS=external_symbolizer_path=foo should already work too. But all the docs mention ASAN_SYMBOLIZER_PATH instead. Should we have LLVM_SYMBOLIZER_PATH in addition to the FOO_OPTIONS=external_symbolizer_path=foo flag that already exists, or is that enough? Should the docs mention that over ASAN_SYMBOLIZER_PATH and MSAN_SYMBOLIZER_PATH? Should the runtime print "use FOO_OPTIONS=external_symbolizer_path=foo instead of ASAN_SYMBOLIZER_PATH" if it's set so that it can eventually be removed?

I can imagine a separate environment variable being more convenient in some cases. I don't think we should deprecate it. Also, printing stuff can break user programs, so it should not be done by default, and doing it only with verbosity=1 is almost useless, no one will see it.

I'd add LLVM_SYMBOLIZER_PATH, maybe add UBSAN_SYMBOLIZER_PATH (just for consistency), and change the docs to point to LLVM_SYMBOLIZER_PATH and external_symbolizer_path.

eugenis accepted this revision.Dec 2 2016, 4:33 PM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 2 2016, 4:33 PM
pcc added a subscriber: pcc.Dec 2 2016, 4:35 PM

I can imagine a separate environment variable being more convenient in some cases. I don't think we should deprecate it. Also, printing stuff can break user programs, so it should not be done by default, and doing it only with verbosity=1 is almost useless, no one will see it.

Maybe only do it if isatty(stdout)?

I'd add LLVM_SYMBOLIZER_PATH, maybe add UBSAN_SYMBOLIZER_PATH (just for consistency), and change the docs to point to LLVM_SYMBOLIZER_PATH and external_symbolizer_path.

Regarding UBSAN_SYMBOLIZER_PATH why add something for consistency with a deprecated env var?

In D27375#612431, @pcc wrote:

I can imagine a separate environment variable being more convenient in some cases. I don't think we should deprecate it. Also, printing stuff can break user programs, so it should not be done by default, and doing it only with verbosity=1 is almost useless, no one will see it.

Maybe only do it if isatty(stdout)?

This could work.

I'd add LLVM_SYMBOLIZER_PATH, maybe add UBSAN_SYMBOLIZER_PATH (just for consistency), and change the docs to point to LLVM_SYMBOLIZER_PATH and external_symbolizer_path.

Regarding UBSAN_SYMBOLIZER_PATH why add something for consistency with a deprecated env var?

That's also true.

thakis closed this revision.Apr 19 2017, 7:17 AM

I was just confused again by this not working, and didn't remember this review for a while. From a user's point of view it feels like this should work, so I've gone ahead and landed this in r300692.