This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Symbolizer refactoring: Make WinSymbolizer use SymbolizerTool interface
ClosedPublic

Authored by kubamracek on Mar 5 2015, 2:10 PM.

Details

Reviewers
samsonov
timurrrr
Summary

Part of http://reviews.llvm.org/D7827. This converts most of WinSymbolizer into WinSymbolizerTool, which uses the common SymbolizerTool. To make the changes more smooth (and patches more readable), I didn't mess up with POSIXSymbolizer, yet. But eventually, I want to "upgrade" POSIXSymbolizer into a cross-platform Symbolizer, that would use WinSymbolizer just as any other symbolizer tool.

For the transition, I made a small class that does the same as POSIXSymbolizer but for Windows.

One issue with the WinSymbolizer is that is uses DbgHelp even for finding module name and offset from an address. I made it a static function, which also needed the initialization functions to be static and the initialized_ field global. The other option would be to add the FindModuleNameAndOffsetForAddress into the SymbolizerTool interface, which however doesn't sound right to me: On POSIX, all the symbolizer tools use the same way to finding module names, so it looks like it's more of a platform-specific thing than a tool-specific one.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 21313.Mar 5 2015, 2:10 PM
kubamracek retitled this revision from to [compiler-rt] Symbolizer refactoring: Make WinSymbolizer use 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 and 2 others.
rnk added a subscriber: timurrrr.Mar 5 2015, 2:24 PM
rnk added a subscriber: rnk.

+timurrrr for WinASan

About FindModuleNameAndOffsetForAddress: Maybe we could simply implement sanitizer_win.cc's GetListOfModules using Win32 APIs EnumProcessModules, GetModuleBaseName and GetModuleInformation. This way we wouldn't require an initialized DbgHelp.dll.

Re: EnumProcessModules -- see also DumpProcessMap() in sanitizer_win.cc.
What's the problem with FindModuleNameAndOffsetForAddress initializing dbghelp?

lib/sanitizer_common/sanitizer_symbolizer_win.cc
29

The implementation is trivial, please make it inline.

156

I think this should be

if (FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
  res->info.FillModuleInfo(module_name, module_offset);
lib/sanitizer_common/sanitizer_symbolizer_win.h
21

Looking at the code in lib/sanitizer_common/sanitizer_symbolizer_internal.h, SymbolizerTool comments assume POSIX. Can you please update the comments there to remove this assumption?

kubamracek updated this revision to Diff 21342.Mar 6 2015, 5:24 AM

Updating patch.

SymbolizerTool comments assume POSIX. Can you please update the comments there to remove this assumption?

I've added a general comment about SymbolizerTool, but currently POSIXSymbolizer is really the only one that implements the chain of symbolizers. After I merge POSIXSymbolizer and WinSymbolizer into just Symbolizer, I will update the comment.

What's the problem with FindModuleNameAndOffsetForAddress initializing dbghelp?

There's no problem, if you're fine with moving the methods and field outside of the WinSymbolizer class. I was just thinking that we might use the same implementation of FindModuleNameAndOffsetForAddress that we have in POSIXSymbolizer, which would simplify sanitizer_symbolizer_win.cc even more (maybe I'll try to do it as a separate patch).

timurrrr edited edge metadata.Mar 6 2015, 5:33 AM

Do you have a Windows box where you can test the patch or do you need help with that?

lib/sanitizer_common/sanitizer_symbolizer_win.cc
26

please rename to is_dbghelp_initialized;

30–34

Please rename to InitializeDbgHelpIfNeeded and move to the top of the file.
Please add a comment that calls to this function should be synchronized, as well as other calls to dbghelp.

You may also consider using an anonymous namespace instead of static and group other dbghelp-related things in that namespace.

60–100

ditto -- please move to the top of the file

150

nit: please add a CHECK that tool is nonzero.

kubamracek updated this revision to Diff 21346.Mar 6 2015, 6:13 AM
kubamracek edited edge metadata.

Addressing review comments.

Do you have a Windows box where you can test the patch or do you need help with that?

Yes, I'm testing the patch on a Windows machine.

timurrrr accepted this revision.Mar 6 2015, 6:20 AM
timurrrr edited edge metadata.

Looks good to me, thanks!

lib/sanitizer_common/sanitizer_symbolizer_win.cc
21

Please make sure this order of includes passes ninja SanitizerLintCheck.
I'm not sure the latter works on Windows, I'm usually doing that on my Linux box before committing.

91

this function should not be placed in the anonymous namespace if is [planned to be] used in other source files.

This revision is now accepted and ready to land.Mar 6 2015, 6:20 AM
kubamracek closed this revision.Mar 6 2015, 6:37 AM

Please make sure this order of includes passes ninja SanitizerLintCheck. I'm not sure the latter works on Windows, I'm usually doing that on my Linux box before committing.

It does pass the lint check.

this function should not be placed in the anonymous namespace

Moved back.

Landed in r231478.