Page MenuHomePhabricator

[lldb] Refactor variable parsing in DWARF symbol file
ClosedPublic

Authored by jarin on Mon, Sep 27, 10:46 AM.

Details

Summary

This is in preparation for a fix of parsing omitted abstract
parameters of inlined functions (see the bug
https://bugs.llvm.org/show_bug.cgi?id=50076#c6 for more details). The
main idea is to move the recursive parsing of variables in a function
context into a separate method (ParseVariablesInFunctionContext).

We also move the setup of block variable list to the DIE node that
starts the block rather than setting up the list when processing
the first variable DIE in that block.

Diff Detail

Event Timeline

jarin created this revision.Mon, Sep 27, 10:46 AM
jarin requested review of this revision.Mon, Sep 27, 10:46 AM

Hi, could you possibly take a look at this change?

The main motivation for this patch is to move the setup of the variable list for parsing children to the DIE node that corresponds to the block containing the children. This will be particularly important for DW_TAG_inlined_subroutine because after a recent clang change (https://reviews.llvm.org/D95617), we can have inlined subroutines which contain no parameters (and thus no children) in the concrete DIE block, but still have some parameters in the abstract function (referenced by the inlined subroutine's DW_AT_abstract_origin attribute). If we started building the variable list when parsing the first variable in a function, we would never create the variable lists for the abstract parameters if they were all omitted.

Note that this change is just a refactoring for the actual change that will introduce parsing of the omitted abstract parameters. The actual change will be uploaded shortly.

shafik added a subscriber: shafik.Mon, Sep 27, 3:08 PM

Just a nitpick.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3603

/*can_create=*/true

3612

/*can_create=*/true

Thank you for taking a look, shafik@. I hope you don't mind if I address your comments later, once a full review arrives from Pavel (or Johannes).

The patch seems reasonable to me, and is an improvement over the status quo.

The thing I'm left wondering is the lambda in ParseVariablesInFunctionContext. It covers like 95% of the enclosing function. Could we just make a separate method for that?

jarin updated this revision to Diff 375535.Tue, Sep 28, 5:23 AM

Separated ParseVariablesInFunctionContext's lambda into a method.

jarin updated this revision to Diff 375538.Tue, Sep 28, 5:30 AM

Added the /*can_create=*/ comments.

jarin added a comment.Tue, Sep 28, 5:34 AM

Thank you for the review! Does the separate method look more reasonable?

labath accepted this revision.Fri, Oct 1, 2:18 AM
This revision is now accepted and ready to land.Fri, Oct 1, 2:18 AM
This revision was automatically updated to reflect the committed changes.
jarin updated this revision to Diff 377474.Wed, Oct 6, 2:02 AM
jarin marked 2 inline comments as done.

Avoid nullptr deref/ref.

jarin added a comment.Wed, Oct 6, 2:13 AM

Pavel, could you take another look? Interestingly the null-deref problem was already fixed in the follow-up patch, maybe I just did not land quickly enough :-)).

labath added a comment.Wed, Oct 6, 3:37 AM

looks good