This is an archive of the discontinued LLVM Phabricator instance.

PostfixExpression: move DWARF generator out of NativePDB internals
ClosedPublic

Authored by labath on Apr 24 2019, 12:59 AM.

Details

Summary

The new dwarf generator is pretty much a verbatim copy of the one in
PDB.

In order to write a pdb-independent test for it, I needed to write a
dummy "symbol resolver", which (together with the fact that I'll need
one more for breakpad-specific resolution logic) prompted me to create a
more simple interface for algorithms which replace or "resolve"
SymbolNodes. The resolving algorithms in NativePDB have been updated to
make use of that too.

I have removed a couple of NativePDB tests which weren't testing
anything pdb-specific and where the tested functionality was covered by
the new format-agnostic tests I have added.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

labath created this revision.Apr 24 2019, 12:59 AM

Can the test deletions be a separate patch, or will they fail because of the other changes in this patch? They feel like a good but separable step.

include/lldb/Symbol/PostfixExpression.h
210

You could leave ref unnamed here to avoid any "unused parameter" warnings.

216

I'm confused why this takes symbol by (non-const) ref. Is symbol modified in the process of figuring out what should replace it?

Oh, peeking ahead at the implementation, I see it can return the address of symbol. I'm left wondering whether there's a less confusing API.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
26

If I understand correctly, we don't care about the return value of Dispatch because all that matters is whether result points to a valid Node or is just nullptr. Right?

If so, then should SymbolResolver derive from Visitor<void> rather than Visitor<bool>?

labath marked 4 inline comments as done.Apr 25 2019, 10:01 AM

Can the test deletions be a separate patch, or will they fail because of the other changes in this patch? They feel like a good but separable step.

Nope, the tests still pass, they just felt superfluous. I'll remove them in a separate patch.

include/lldb/Symbol/PostfixExpression.h
216

Yeah, I too had the feeling that there ought to be a simpler way to do this, but I couldn't figure out how.... until now...

Check out the new version of the patch. Now, instead of having to subclass something one can implement a trivial search-and-replace operation by just passing a callback to the function. (There is subclassing going on under the hood, but the user is unaware of that).

In fact, it made things so simple, I just decided to merge the two-pass algorithm in NativePDB into a single function.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
26

Not exactly. The fact whether is the replacement function returned nullptr is then propagated up the Dispatch function, and eventually makes its way out as the result of the entire replacement operation (so, yes, the bool is needed here).

labath updated this revision to Diff 196665.Apr 25 2019, 10:02 AM
labath marked an inline comment as done.
  • simplify symbol resolution even more
  • drop the NativePDB unittest changes
amccarth accepted this revision.Apr 25 2019, 11:04 AM
amccarth marked 2 inline comments as done.

Clever (hopefully not too clever)! Not having to derive a special class from Visitor is really nice.

LGTM.

include/lldb/Symbol/PostfixExpression.h
216

Well, it is simpler, but we still have that non-const ref to symbol, which is what originally threw me. I don't see a way to get around that, and I like the new solution better than having to derive your own visitor.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
26

Oh yes. I was looking at the Dispatch calls in the DWARF CodeGen class by mistake. Those are Visitor<void>. Perfect.

This revision is now accepted and ready to land.Apr 25 2019, 11:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 1:51 AM