Let ubsan search UBSAN_SYMBOLIZER_PATH for llvm-symbolizer

[thakis]

Summary

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

[eugenis]

how about adding LLVM_SYMBOLIZER_PATH that works in all sanitizers?

[thakis]

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?

[eugenis]

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]

LGTM

[pcc]

In D27375#612426, @eugenis 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)?

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?

[eugenis]

In D27375#612431, @pcc wrote:

In D27375#612426, @eugenis 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]

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.

lib/ubsan/ubsan_flags.cc

 }
 
 void InitializeFlags() {
   SetCommonFlagsDefaults();
   {
     CommonFlags cf;
     cf.CopyFrom(*common_flags());
     cf.print_summary = false;
+    cf.external_symbolizer_path = GetEnv("UBSAN_SYMBOLIZER_PATH");
     OverrideCommonFlags(cf);
   }
 
   Flags *f = flags();
   f->SetDefaults();
 
   FlagParser parser;
   RegisterCommonFlags(&parser);