Page MenuHomePhabricator

Breakpad: Generate unwind plans from STACK CFI records
ClosedPublic

Authored by labath on May 9 2019, 7:20 AM.

Details

Summary

This patch implements the GetUnwindPlan interface (added in the previous
patch) for SymbolFileBreakpad, and uses it to generate unwind plans from
STACK CFI records in breakpad files.

We first perform a light-weight parse of the breakpad in order to build
up a map of regions covered by the unwind info so that we can later jump
to the right record when we need to unwind a specific function.

The actual parsing is relatively straight-forward, as the STACK CFI records
are just another (text) form of the eh_frame unwind instructions, and
the same goes for lldb's UnwindPlans. The newly-introduced
PostfixExpression API is used to convert the breakpad postfix
expressions into DWARF. The generated dwarf expressions are stored in a
BumpPtrAllocator, as the UnwindPlan does not take ownership of the
expression data it references (usually this is static data in an object
file, so special ownership is needed).

At this moment the generated unwind plans aren't used in the actual
unwind machinery (only in the image show-unwind command), but that is
coming in a separate patch.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

labath created this revision.May 9 2019, 7:20 AM
clayborg added inline comments.May 9 2019, 7:31 AM
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
378

This format seems a bit challenging to parse

418

LLDB_REGNUM_GENERIC_RA? Do we want the PC here or do we want the link register?

491–492

will this code be ok if the assertions are compiled out?

labath marked 4 inline comments as done.May 9 2019, 9:47 AM
labath added inline comments.
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
418

It looks a bit weird, but I believe it should be the PC (and I have checked that things unwind correctly this way), because we are specifying value for the PC in the parent frame (which is the same as the return address of the current frame). Or, to put it another way, breakpad uses ".ra" even on platforms which do not have a LLDB_REGNUM_GENERIC_RA register (like x86).

491–492

Yes, because we make sure that we only store the address of a syntactically correct STACK CFI INIT record in the m_unwind_data map (down on line 645)

clayborg accepted this revision.May 9 2019, 10:04 AM

Thanks for the clarification! LGTM

This revision is now accepted and ready to land.May 9 2019, 10:04 AM
amccarth accepted this revision.May 9 2019, 1:03 PM

LGTM once you double-check the return value in the error case at the end of SymbolFileBreakpad::ParseUnwindRow.

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
472

Should the function return false when there are leftover unparsable rules? The other error cases seem to do that.

jasonmolenda accepted this revision.May 9 2019, 5:32 PM
jasonmolenda added inline comments.
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
418

This is fine, fwiw RegisterContextLLDB won't try to use LLDB_REGNUM_GENERIC_RA on architectures that don't use a return address register (e.g. lr on arm), so using LLDB_REGNUM_GENERIC_PC is correct if we ever need to do this unwind on an x86 breakpad file.

labath marked 3 inline comments as done.May 10 2019, 2:25 AM
labath added inline comments.
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
472

Good point. In fact this should have been caught by the test, if it was written properly, but I forgot the colon after the CHECK-NOT directive, which meant it was ignored by FileCheck. :/
Fixing that found another case where I was mistakenly returning a "valid" unwind plan for invalid input, which I have fixed too.

labath updated this revision to Diff 198993.May 10 2019, 2:28 AM
  • use a yaml form of the minidump file (as it is possible to do that as of today)
  • fix the CHECK-NOT directives in the test and the two issues they caught
  • found a way to trigger one of the asserts with an invalid input, so I demote it into a "return nullptr"
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2019, 4:24 AM
Herald added a subscriber: abidh. · View Herald Transcript