Page MenuHomePhabricator

Breakpad: Basic support for STACK WIN unwinding
ClosedPublic

Authored by labath on Sep 2 2019, 2:40 AM.

Details

Summary

This patch makes it possible to unwind via breakpad STACK WIN records.
It is "basic" because two important features are missing:

  • support for the .raSearch keyword
  • support for multiple STACK WIN records within a single function

Right now, we just reject the .raSearch records, and always pick the
first record for the whole function
SymbolFileBreakpad, and so I think it can serve as a good example of
what is needed of the symbol file and unwinding machinery to make this
work.

However, it is already useful for unwinding in some situations, and it
sets up the general framework for the parsing of these kinds of records,
which reduces the size of the followup patches implementing the two
other components.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Sep 2 2019, 2:40 AM

This is looking pretty good.

I'm wondering whether it needs some "negative" tests.

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
549 ↗(On Diff #218316)

Should we log and bail out rather than just assert? A corrupt symbol file shouldn't kill the debugger, right?

Also, it's Optional rather than Expected, so it seems even more plausible to hit this.

586 ↗(On Diff #218316)

Typo: realign

labath updated this revision to Diff 218611.Sep 4 2019, 1:50 AM
labath marked 3 inline comments as done.

add more tests

labath added a comment.Sep 4 2019, 1:50 AM

We always need more tests. I've now added more tests for various boundary conditions.

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
549 ↗(On Diff #218316)

This is an internal consistency check. An entry will be added to the m_unwind_data->win map only if it was already parsed successfully down in ParseUnwindData. This is parsing the same data once more, so it should always succeed.

Now the next question is, why parse the same data twice? :)
The first parse is to build an index of the ranges covered by the breakpad file. In the second pass we actually parse the undwind data. Theoretically we could avoid the second parse if we stored more data in the first one. However, here I am operating under the assumption that most record will not be touched, so it's worth to save some space for *each* record for the price of having to parse twice *some* of them. This seems like a good tradeoff intuitively, but I am don't have hard data to back that up.

Also, the case was much stronger for STACK CFI records (which do the same thing), as there I only have to put the STACK CFI INIT records into the map (and each INIT record is followed by a potentially large number of non-INIT records). STACK WIN records don't have an INIT records, so I have to insert all of them anyway, which makes the savings smaller, but it still seems worth it.

amccarth accepted this revision.Sep 4 2019, 9:19 AM

LGTM. Please consider adding a comment to the assert that summarizes what you explained in the thread.

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
549 ↗(On Diff #218316)

Cool. Could you just add a comment at the assertion that says a short version of that. For example, "We've already parsed it once, so it shouldn't fail this time."

This revision is now accepted and ready to land.Sep 4 2019, 9:19 AM
labath marked an inline comment as done.Sep 5 2019, 12:04 AM
labath added inline comments.
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
549 ↗(On Diff #218316)

Will do. I'll also add the comment to the STACK CFI version of this function.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 12:05 AM