Page MenuHomePhabricator

[compiler-rt] Symbolizer refactoring: Unify access to symbolizer tools from POSIXSymbolizer
ClosedPublic

Authored by kubamracek on Mar 3 2015, 2:49 AM.

Details

Reviewers
samsonov
Summary

Part of http://reviews.llvm.org/D7827. The way we select symbolizer tools in sanitizer_symbolizer_posix_libcdep.cc's Symbolizer::PlatformInit currently uses only one (or zero) of them. And since all the POSIX symbolizers now use the commmon interface, we don't need to special-case the three classes anymore.

To maintain the current behavior, I changed the Demangle method in SymbolizerTool interface to return nullptr by default, which means that POSIXSymbolizer will fallback to use DemangleCXXABI. And in SymbolizeData of POSIXSymbolizer, the addition of the module base address (info->start += module->base_address()) has been moved into the individual tools, because not all tool parse back a relative offset (libbacktrace already fills in the full address).

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 21090.Mar 3 2015, 2:49 AM
kubamracek retitled this revision from to [compiler-rt] Symbolizer refactoring: Unify access to symbolizer tools from POSIXSymbolizer.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), zaks.anna, samsonov.

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

The initialization of the external symbolizer (in PlatformInit) is wrapped inside if (!libbacktrace_symbolizer), so it doesn't look to me that we currently have such a fallback. Or am I reading the code wrong?

Anyway, I definitely support having fallbacks, and I plan to submit another patch that implements the chain of symbolizers.

In D8029#133842, @kubabrecka wrote:

I'm happy with the direction of this change, although:

Note that this does introduce a change in behavior - in case both libbacktrace *and* external symbolizer are available, we could fallback from former to latter. Is there a strong reason for removing this fallback? I originally thought that it can be useful in symbolizers you were going to add.

The initialization of the external symbolizer (in PlatformInit) is wrapped inside if (!libbacktrace_symbolizer), so it doesn't look to me that we currently have such a fallback. Or am I reading the code wrong?

Ah, you are right. Feel free to submit this change first if it's convenient for you.

Anyway, I definitely support having fallbacks, and I plan to submit another patch that implements the chain of symbolizers.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
481–482

I would re-write this function using early returns:

static SymbolizerTool *ChooseSymbolizer() {
  if (!common_flags()->symbolize)
    return nullptr;
  if (SymbolizerTool *tool = InternalSymbolizer::get(..)) {
    return tool;
  }
  if (SymbolizerTool *tool = LiibacktraceSymbolizer::get(..)) {
    return tool;
  }
  <...>
}

and then have a single `new(symbolizer_allocator_) POSIXSymbolizer(ChooseSymbolizer());`

kubamracek updated this revision to Diff 21148.Mar 3 2015, 2:52 PM

Updating patch.

samsonov accepted this revision.Mar 3 2015, 3:00 PM
samsonov added a reviewer: samsonov.

LGTM. Do you plan to inherit WinSymbolizer from SymbolizerTool?

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
490

Restore the comment here (external symbolizer is explicitly disabled).

This revision is now accepted and ready to land.Mar 3 2015, 3:00 PM
kubamracek closed this revision.Mar 3 2015, 3:26 PM

Landed in r231162.

LGTM. Do you plan to inherit WinSymbolizer from SymbolizerTool?

Yes, although it's not as straight-forward as I'd like. I planned to move the implementation of POSIXSymbolizer into just Symbolizer, which would no longer have any subclasses. The ChooseSymbolizer would then simply have a if (SANITIZER_WINDOWS) branch in it, and we wouldn't need PlatformInit at all.

Speaking of Windows, I found out that we currently don't run the testcases from the main TestCases/ directory on Windows (we treat them as if they were in TestCases/Posix/), is that intentional? If yes, what's the point of having a separate Posix subdirectory?