Page MenuHomePhabricator

[compiler-rt] Add AtosSymbolizer and DlAddrSymbolizer as fallbacks for OS X
ClosedPublic

Authored by kubamracek on Mar 12 2015, 4:38 AM.

Details

Reviewers
samsonov
Summary

This patch changes the symbolizer chain on OS X (which currently only uses 1 symbolizer at most) to use this behavior:

  • By default, use LLVMSymbolizer -> AtosSymbolizer -> DlAddrSymbolizer.
  • If the llvm-symbolizer binary is not found, use AtosSymbolizer -> DlAddrSymbolizer.
  • If the user specifies ASAN_SYMBOLIZER_PATH=.../atos, then use AtosSymbolizer -> DlAddrSymbolizer.

There are several reasons for these fallbacks, e.g. when fork is disabled or when the llvm-symbolizer is just not present or found on the system.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 21816.Mar 12 2015, 4:38 AM
kubamracek retitled this revision from to [compiler-rt] Add AtosSymbolizer and DlAddrSymbolizer as fallbacks for OS X.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), zaks.anna, samsonov and 2 others.

The logic here is very convoluted. First of all, I don't like that you ripped parts of ChooseMainSymbolizerTools out of this function, and it now doesn't do what it says. Let's keep all this logic contained. Now, more questions:

  1. would you need to fallback to DlAddrSymbolizer if internal/libbacktrace is available (I know that for now they are not available on Mac, but still). I don't think so.
  2. do you need to fallback to atos if the user *explicitly* specified llvm-symbolizer in ASAN_SYMBOLIZER_PATH?
  3. do you need to fallback to atos if llvm-symbolizer was detected by FindPathToBinary call?

That is, I'd first like to figure out if there's a case when we need more than one out-of-process symbolizer. If not, we can restructure the code like this (pseudocode):

if (path_to_external && path_to_external[0] == '\0') { /* add nothing */ }
else if (EndsWith(path_to_external, "llvm-symbolizer(.exe)?") { /* use llvm-symbolizer */ }
else if (EndsWith(path_to_external, "atos")) { /* use atos */ }
else if (EndsWith(path_to_external, "addr2line")) { /* use addr2line */ }
else {
  assert(path_to_external == nullptr);  // Otherwise symbolizer program is unknown
  /* choose which one of llvm-symbolizer, atos, or addr2line is available and use it */
}

You can move *this* piece (figuring out external symbolizer to use) in a separate function.

else if (EndsWith(path_to_external, "llvm-symbolizer(.exe)?") { /* use llvm-symbolizer */ }

Is llvm-symbolizer.exe really a thing? I though on Windows we only support the DbgHelp symbolizer.

kubamracek updated this revision to Diff 21928.Mar 13 2015, 7:11 AM

Updating the patch. Extracted a ChooseExternalSymbolizer which selects one external symbolizer that will be used (I guess having multiple external symbolizers doesn't make sense).

samsonov added inline comments.Mar 19 2015, 4:31 PM
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
421

return nullptr here

431

Just write Die() here.

440

Die()

442

No need for else{}

kubamracek updated this revision to Diff 22331.Mar 19 2015, 6:06 PM

Updating patch.

samsonov edited edge metadata.Mar 20 2015, 11:46 AM

Looks reasoable. Is sandbox-exec available on all Mac OS X systems?

test/asan/TestCases/Darwin/dyld-root-path.cc
8 ↗(On Diff #22331)

Please merge this test case into test/asan/TestCases/Darwin/atos-symbolizer.cc

test/asan/TestCases/Darwin/sandbox-symbolizer.cc
13

You can reduce the number of compilations in this test:
first compile with -O0, then run two tests with sandbox-exec, then compile with -O3, then run two more tests with sandbox-exec.

kubamracek updated this revision to Diff 22412.Mar 21 2015, 7:26 AM
kubamracek edited edge metadata.

Addressing review comments.

Is sandbox-exec available on all Mac OS X systems?

Yes, sandbox-exec is available on OS X versions that sanitizers are supposed to work on (10.7+).

samsonov accepted this revision.Mar 21 2015, 8:18 PM
samsonov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 21 2015, 8:18 PM
kubamracek closed this revision.Mar 22 2015, 3:06 AM

Landed in r232908. Thanks for the review and your patience!

This change appears to have had the unfortunate effect that setting ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer-3.8 in the environment no longer works, because the strcmp on line 423 is looking for the exact string 'llvm-symbolizer'.

That is unfortunate, since on some Linux distributions like Ubuntu the llvm-symbolizer binaries are always installed with a version suffix. On my Ubuntu 16.04 system, for instance, the symbolizer is in /usr/bin/llvm-symbolizer-3.8. Adjusting PATH doesn't help in that case, for obvious reasons. So it appears that after the above change the only way to deal with this on Ubuntu is to make a temporary copy of /usr/bin/llvm-symbolizer-3.8 to a temp directory under the name llvm-symbolizer, and then either add that to PATH or point ASAN_SYMBOLIZER_PATH at the temporary llvm-symbolizer file.

If this was an intended behavior change, then OK, but if unintended it feels like a regression in 3.7 and 3.8.