Page MenuHomePhabricator

[compiler-rt] Symbolizer refactoring: Merge common parts of POSIXSymbolizer and WinSymbolizer
ClosedPublic

Authored by kubamracek on Mar 6 2015, 7:30 AM.

Details

Reviewers
samsonov
Summary

Part of http://reviews.llvm.org/D7827. Moves most of the implementation from POSIXSymbolizer to the superclass, Symbolizer, and reuses that in WinSymbolizer. Removes Symbolizer::Disable(), because we're not using it anyway.

Note that the public methods of Symbolizer lock the mu_ mutex, so both the tool's methods *and* the platform-specific virtual methods of Symbolizer (such as FindModuleNameAndOffsetForAddress) are synchronized.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 21354.Mar 6 2015, 7:30 AM
kubamracek retitled this revision from to [compiler-rt] Symbolizer refactoring: Merge common parts of POSIXSymbolizer and WinSymbolizer.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), zaks.anna, samsonov and 4 others.

Deferring to Alexey.

Some comments inline:

lib/sanitizer_common/sanitizer_symbolizer.cc
98

Interesting: is it possible for a PC to not belong to any module, but still be symbolizable by SymbolizerTools? JITed code maybe?

lib/sanitizer_common/sanitizer_symbolizer.h
127

I encourage you to write a comment explaining that mu_ not only synchronizes calls to Symbolizer, but should actually be used to synchronize all the calls to the underlying symbolizers (e.g. dbghelp on Windows). Not sure where's the best place to put this comment.

Random thought: I wonder if it's time to create a new lib/sanitizer_common/symbolization dir?..

is it possible for a PC to not belong to any module, but still be symbolizable by SymbolizerTools

Good question. We certainly don't try to do that in the current POSIXSymbolizer (for any of the POSIX tools). Do you know if DbgHelp can do this?

I encourage you to write a comment explaining that mu_ not only synchronizes calls to Symbolizer, but should actually be used to synchronize all the calls to the underlying symbolizers (e.g. dbghelp on Windows).

I forgot to say that I still plan to get rid of POSIXSymbolizer and WinSymbolizer subclasses completely, moving the platform-specific parts elsewhere. But in the meantime, I agree the comment should be there.

timurrrr added inline comments.
lib/sanitizer_common/sanitizer_symbolizer.cc
98

[you might consider replying using the web interface, otherwise the discussion is a bit hard to follow here]

Good question. We certainly don't try to do that in the current POSIXSymbolizer
(for any of the POSIX tools). Do you know if DbgHelp can do this?

I don't know actually, neither do I know if we can use DIA SDK for that... But I can imagine us adding support for plugin-line symbolizers, e.g. for V8-generated code maybe?

Kostya, Zachary, any ideas?

samsonov edited edge metadata.Mar 6 2015, 4:54 PM

Looks mostly good.

lib/sanitizer_common/sanitizer_symbolizer.h
88

run clang-format over the changed lines.

96

This function is short, you can just inline it here.

117

This is also Platform-specific function, might make sense to reflect it in the name.

127

Right, just add a comment here.
Timur: I'd rather not add extra directory for that - that would make build rules more complex.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
357

no need for virtual here.

kubamracek updated this revision to Diff 21420.Mar 7 2015, 3:04 AM
kubamracek edited edge metadata.

Addressing review comments.

samsonov accepted this revision.Mar 9 2015, 11:10 AM
samsonov edited edge metadata.

LGTM. Please watch the bots after commit.

This revision is now accepted and ready to land.Mar 9 2015, 11:10 AM
kubamracek closed this revision.Mar 9 2015, 12:08 PM

Landed in r231680.