Page MenuHomePhabricator

Postfix: move more code out of the PDB plugin
ClosedPublic

Authored by labath on Aug 23 2019, 2:42 AM.

Details

Summary

Previously we moved the code which parses a single expression out of the PDB
plugin, because that was useful for DWARF expressions in breakpad. However, FPO
programs are used in breakpad files too (when unwinding on windows), so this
completes the job, and moves the rest of the FPO parser too.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Aug 23 2019, 2:42 AM

LGTM, thanks!

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
58 ↗(On Diff #216789)

Do I understand right, you use a vector of pairs instead of a map due to the small number of expressions in a program (then a search on a small vector will be faster than on a map)?

This revision is now accepted and ready to land.Aug 25 2019, 11:39 PM
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 4:43 AM

I have a couple inline questions. After that, it looks fine.

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
61

I would recommend making parsed a const vector. The lambda captures the iterator, and while the code currently looks fine, I'd hate for something to change in the future that could invalidate the captured iterator.

69

This looks O(n^2). Will that matter here or will the expressions always be short?

84

It's a shame the pair loses the more descriptive field names, but I don't see a good way to make it better, so this is a useless comment.

lldb/trunk/source/Symbol/PostfixExpression.cpp
101

Is it correct to return an empty vector here rather than result? If there's one bad expression, you'll consider the entire FPO program empty. That's sounds plausible, but I thought I'd ask.

labath marked 3 inline comments as done.Aug 27 2019, 1:54 AM

Please find my replies inline (including my yesterday's reply to Aleksandr, which I apparently failed to submit).

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
61

Making it const won't work because the symbol resolution process can actually modify the contents (not the size) of the vector -- if the RHS is a SymbolNode * directly, then we may need to replace that with a different kind of a node.

I don't think that the invalidation problem is really specific to the lambda -- even without the lambda, if anything modified the vector size while we were iterating over it, we would blow up.

69

I don't think that should matter, the expressions will typically have three of four assignments (frame base, aligned frame base, esp, eip). It may have additional entries for spilled registers, but there aren't that many of those either.

lldb/trunk/source/Symbol/PostfixExpression.cpp
101

Yeah, that was intentional here. If there's an error parsing one of the expressions, I am not sure we want to trust the parse of the preceding partial results either. I wouldn't really use the term "correct" here, because normally we should never fail to parse the expression, and the failure would either mean a bug in the parser, or the producer. And it's hard to say what is the "correct" thing to do in those cases without knowing the specifics...

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
58 ↗(On Diff #216789)

Not exactly. The main reason for using a vector is that the order of expressions (according to my understanding) in these expressions is important. So a b = b c = would mean something different than b c = a b =. And once I already have them in a vector, I didn't see a point in putting them also into a map to get fast lookups (because the number of elements is small).

The previous implementation avoided that by parsing and resolving in the same loop, but here I first parse and then resolve stuff. This made it easier to implement, and I doubt the performance loss is noticeable due to the size of the data involved.

When I added my comments, Phabricator showed this patch as still open. Now it looks like it landed four hours before that. :-(

When I added my comments, Phabricator showed this patch as still open. Now it looks like it landed four hours before that. :-(

Yeah, :(, but that doesn't mean I can't address any comments in a follow-up. If you have any additional feedback, or thoughts on my previous comments, let me know.