Page MenuHomePhabricator

Make llvm-symbolizer work on Windows.
ClosedPublic

Authored by zturner on Apr 23 2015, 2:39 PM.

Details

Summary

This is a rework of my original patch which introduced a new class hierarchy into llvm-symbolizer. Now that DIContext has moved to a common layer, this patch simply uses DIContext, resulting in much nicer code in llvm-symbolizer.

Inline frames are not yet supported, so we fall back to just pretending like the option wasn't specified whenever a PDBContext is in use. Test program:

Here is some program output showing that it works:

d:\testexe>cat test.c
  void foo() {
}

int main() {
  foo();
}

d:\testexe>dumpbin /DISASM test.exe | grep "_foo:\|_main:" --after-context=1
_foo:
  00401020: 55                 push        ebp
--
_main:
  00401030: 55                 push        ebp

d:\testexe>d:\src\llvmbuild\ninja\bin\llvm-symbolizer.exe --obj test.exe
0x401020
_foo
d:\testexe\test.c:1:0

0x401030
_main
d:\testexe\test.c:4:0

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 24335.Apr 23 2015, 2:39 PM
zturner retitled this revision from to Make llvm-symbolizer work on Windows..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: samsonov, timurrrr.
zturner updated this object.
zturner added a subscriber: Unknown Object (MLST).
samsonov added inline comments.Apr 23 2015, 6:12 PM
include/llvm/DebugInfo/PDB/PDBContext.h
10 ↗(On Diff #24335)

Header guard looks wrong.

30 ↗(On Diff #24335)

Will these be implicitly deleted anyway?

lib/DebugInfo/PDB/PDBContext.cpp
37 ↗(On Diff #24335)

Note that ImageBase may be uninitialized at this point.

60 ↗(On Diff #24335)

Can LineInfo be nullptr at this point?

tools/llvm-symbolizer/LLVMSymbolize.cpp
169 ↗(On Diff #24335)

The code has assumption that ModuleInfo::symbolizeInlinedCode() always returns *some* frames (at least one empty) - see the special case for InlinedContext.getNumberOfFrames() == 0. Hence, the assert in LLVMSymbolizer::symbolizeCode().

I think, that's a wrong place to add a fallback - instead you can make PDBContext::getInliningInfoForAddress() return a DIInliningInfo with a single frame, obtained from PDBContext::getLineInfoForAddress().

tools/llvm-symbolizer/llvm-symbolizer.cpp
131 ↗(On Diff #24335)

Can you create RAII object that would call CoInitializeEx in ctor and CoUninitialize in dtor (and use it in applicable tools - for now llvm-symbolizer / llvm-dwarfdump)? It can have empty implementation on non-Windows platforms.

timurrrr edited edge metadata.Apr 24 2015, 10:03 AM

It's great to see how small the changes to the symbolizer code are!

I'll defer to Alexey for the review as I'm not at all familiar with the code.

include/llvm/DebugInfo/PDB/PDBContext.h
23 ↗(On Diff #24335)

nit: s/pdb/PDB/

lib/DebugInfo/PDB/PDBContext.cpp
78 ↗(On Diff #24335)

can this return nullptr?

tools/llvm-symbolizer/LLVMSymbolize.cpp
472 ↗(On Diff #24335)

nit:
if (X *x = dyn_cast<X>(...)) { (1)
is not consistent with the other parts of the change where you use
if (auto x = dyn_cast<X>(...)) {

Personally I like the former (1) more.

zturner updated this revision to Diff 24406.Apr 24 2015, 11:26 AM
zturner edited edge metadata.

I've addressed all of the comments except for the RAII com initialization comment. I think that can happen as a separate CL, since it will need to undergo review from others to make sure they're comfortable with the way we're introducing it (since COM is kind of a platform-specific thing, it has to be done carefully).

samsonov edited edge metadata.Apr 24 2015, 12:05 PM

Can you add tests for this functionality (proper handling of invalid addresses etc.)? For DWARF, we just check in small binaries with debug information (see test/DebugInfo/llvm-symbolizer.test).

include/llvm/DebugInfo/PDB/PDBContext.h
10 ↗(On Diff #24406)

LLVM_DEBUGINFO_PDB_PDBCONTEXT_H ?

(yep, I've just noticed that header guards in include/llvm/DebugInfo/DWARF/* files are wrong, I will probably fix that.

lib/DebugInfo/PDB/PDBContext.cpp
44 ↗(On Diff #24406)

What if Symbol is nullptr? Probably you should bail out early in this case.

57 ↗(On Diff #24406)

You can reduce indentation with

if (Length == 0)
  return Result;
61 ↗(On Diff #24406)

if LineInfo can't be nullptr, can we assert on this?

87 ↗(On Diff #24406)

Looks like you can move copying the contents of LineInfo (from IPDBEnumLineNumbers) to DILineInfo into a helper function.

94 ↗(On Diff #24406)

Should this be dyn_cast_or_null ?

zturner updated this revision to Diff 24415.Apr 24 2015, 2:37 PM
zturner edited edge metadata.

Fix remaining issues and added a test.

samsonov accepted this revision.Apr 24 2015, 3:39 PM
samsonov edited edge metadata.

LGTM. There are a few minor comments below, feel free to land after addressing them. Thank you!

test/tools/llvm-symbolizer/pdb/Inputs/addresses.txt
1 ↗(On Diff #24415)

Probably filename should reflect that these are addresses in test.exe

4 ↗(On Diff #24415)

missing newline?

test/tools/llvm-symbolizer/pdb/Inputs/test.c
1 ↗(On Diff #24415)

Could you provide comment with brief instruction on how to build test.exe / test.pdf from it?

test/tools/llvm-symbolizer/pdb/pdb.test
9 ↗(On Diff #24415)

missing newline

tools/llvm-symbolizer/llvm-symbolizer.cpp
20 ↗(On Diff #24415)

config.h is probably not needed now?

This revision is now accepted and ready to land.Apr 24 2015, 3:39 PM
This revision was automatically updated to reflect the committed changes.