This is an archive of the discontinued LLVM Phabricator instance.

[Windows] Symbolize with llvm-symbolizer instead of dbghelp in a self-host
ClosedPublic

Authored by rnk on Sep 15 2015, 11:32 AM.

Details

Summary

llvm-symbolizer understands both PDBs and DWARF, so it is more likely to
succeed at symbolization. If llvm-symbolizer is unavailable, we will
fall back to dbghelp. This also makes our crash traces more similar
between Windows and Linux.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 34817.Sep 15 2015, 11:32 AM
rnk retitled this revision from to [Windows] Symbolize with llvm-symbolizer instead of dbghelp in a self-host.
rnk updated this object.
rnk added reviewers: Bigcheese, zturner, chapuni.
rnk added a subscriber: llvm-commits.
zturner added inline comments.Sep 15 2015, 11:40 AM
lib/Support/Signals.cpp
99–100 ↗(On Diff #34817)

Does createTemporaryFile not open the file in such a way that it has FILE_FLAG_DELETE_ON_CLOSE or the equivalent on unix platforms?

138 ↗(On Diff #34817)

os << format_hex() already behaves consistently. It's not a big deal, but it seems better to use type-safe formatting whenever possible, and I think format_hex would work just fine in that regard here.

lib/Support/Windows/Signals.inc
282–283 ↗(On Diff #34817)

Seems like this code could move after the #ifdef, no need to initialize anything if we end up not using Dbghelp anyway

289–292 ↗(On Diff #34817)

Why only when self-hosting? Wouldn't it be worth it to try llvm-symbolizer no matter what?

zturner resigned from this revision.Oct 15 2015, 1:48 PM
zturner removed a reviewer: zturner.
rnk added inline comments.Nov 4 2015, 2:30 PM
lib/Support/Signals.cpp
99–100 ↗(On Diff #34817)

Apparently not. =/

138 ↗(On Diff #34817)

Cool, I switched to that.

lib/Support/Windows/Signals.inc
282–283 ↗(On Diff #34817)

StackWalk64 is part of dbghelp, and we need to initialize it first.

289–292 ↗(On Diff #34817)

Yeah, I'll get rid of that. My idea was to avoid changing behavior for a standard Windows build, but we might as well do it.

rnk updated this revision to Diff 39263.Nov 4 2015, 2:48 PM
  • Use format_hex instead of format()
Bigcheese accepted this revision.Nov 4 2015, 5:03 PM
Bigcheese edited edge metadata.
This revision is now accepted and ready to land.Nov 4 2015, 5:03 PM
This revision was automatically updated to reflect the committed changes.