This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Symbolizer refactoring: Make LibbacktraceSymbolizer adopt the SymbolizerTool interface
ClosedPublic

Authored by kubamracek on Feb 28 2015, 2:16 AM.

Details

Reviewers
samsonov
Summary

Part of http://reviews.llvm.org/D7827. This converts LibbacktraceSymbolizer to adopt the new SymbolizerTool interface that all symbolizers will (eventually) use.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 20931.Feb 28 2015, 2:16 AM
kubamracek retitled this revision from to [compiler-rt] Symbolizer refactoring: Make LibbacktraceSymbolizer adopt the SymbolizerTool interface.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), zaks.anna, samsonov.

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc
94

last can't be nullptr now, perhaps you can add a CHECK. Or, better, as this function is only used once, you just inline it in get_new_frame

104

I'd prefer to have

CHECK(last);
if (frames_symbolized > 0) {
    SymbolizedStack *cur = SymbolizedStack::New(addr);
    /.../
    last->next = cur;
    last = cur;
}
CHECK_EQ(addr, last->info.address);
return &last->info;
179

Shouldn't you return false if data.frames_symbolized is zero?

205

Make it a static function.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
570

Looks like you can rewrite it now:

SymbolizeStack *res = SymbolizedStack::New(addr);
if (!FindModuleNameAndOffset(...))
  return res;
res->info.FillAddressAndModuleInfo(addr, module_name, module_offset);
<...>
kubamracek updated this revision to Diff 21036.Mar 2 2015, 2:03 PM

Updating patch.

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

I'll do it in a subsequent patch.

samsonov accepted this revision.Mar 2 2015, 2:14 PM
samsonov added a reviewer: samsonov.

LGTM

On a related note, let me know if you want to replace AddressInfo::FillAddressAndModuleInfo with AddressInfo::FillModuleInfo, or I should do it myself.

I'll do it in a subsequent patch.

Thanks!

lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc
101

Should this be first->info.address?

This revision is now accepted and ready to land.Mar 2 2015, 2:14 PM

CHECK_EQ(addr, last->info.address);
Should this be first->info.address?

For llvm-symbolizer, we always have the same addr in all frames coming from one SymbolizePC(addr) request. I am assuming the same holds for libbacktrace symbolizer. If that's really the case, I'd even vote for having both checks (addr == first->info.address == last->info.address).

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

In D7971#132931, @kubabrecka wrote:

CHECK_EQ(addr, last->info.address);
Should this be first->info.address?

For llvm-symbolizer, we always have the same addr in all frames coming from one SymbolizePC(addr) request. I am assuming the same holds for libbacktrace symbolizer. If that's really the case, I'd even vote for having both checks (addr == first->info.address == last->info.address).

Makes sense, let's have both checks.

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

No, currently it's only used in GCC. Did you build it with it this definition?

Btw. do we have any buildbots that is at least building with SANITIZER_LIBBACKTRACE=1?

No, currently it's only used in GCC. Did you build it with it this definition?

Yes, I did. But it seems to me it doesn't build out-of-the-box with dynamic runtime – it failed to link and I needed to modify CMake definitions to add -lbacktrace into the .so link phase. Are there official steps how to build and use libbacktrace symbolizer?

kubamracek closed this revision.Mar 2 2015, 3:09 PM

Landed in r231032.