Page MenuHomePhabricator

[lldb/DWARF] Fix handling of variables with both location and const_value attributes
ClosedPublic

Authored by labath on Aug 26 2020, 5:33 AM.

Details

Summary

Class-level static constexpr variables can have both DW_AT_const_value
(in the "declaration") and a DW_AT_location (in the "definition")
attributes. Our code was trying to handle this, but it was brittle and
hard to follow (and broken) because it was processing the attributes in
the order in which they were found.

Refactor the code to make the intent clearer -- DW_AT_location trumps
DW_AT_const_value, and fix the bug which meant that we were not
displaying these variables properly (the culprit was the delayed parsing
of the const_value attribute due to a need to fetch the variable type.

Diff Detail

Event Timeline

labath created this revision.Aug 26 2020, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 5:33 AM
labath requested review of this revision.Aug 26 2020, 5:33 AM
aprantl accepted this revision.Aug 26 2020, 9:38 AM

What a function :-)

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

Would be nice to early-exit here, too.

3229

If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would make sense...

This revision is now accepted and ready to land.Aug 26 2020, 9:38 AM
shafik accepted this revision.Aug 26 2020, 3:12 PM

LGTM with minor comments. Thank you for these fixes, they are awesome!

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

It might be helpful to document that DW_AT_location is taken over DW_AT_const_value and the types of cases this can show up in.

3188

const DWARFDataExtractor&?

I don't think auto adds any value here.

3213

const DWARFDataExtractor&?

3229

Why +1?

3421–3422

make_shared

aprantl added inline comments.Aug 26 2020, 6:44 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3229

The NUL-terminator?

labath marked 3 inline comments as done.Aug 27 2020, 6:02 AM
labath added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3106

I'll do that in a separate patch.

3229

Yeah, this is supposed to create a memory view of a C string, so it (probably) needs to include the nul terminator. But that actually speaks against the AsCStringRef function, as I wouldn't expect that one to include the null terminator.

I say "probably" because I haven't been able to actually find a producer that would produce this kind of attribute. The closest I got was gfortran, which could emit emit DW_AT_const_value for string variables. However, it used block forms for that.

To my great surprise, my synthetic test case in that patch actually worked and using a string DW_AT_const_value for char arrays in C seems somewhat reasonable, so I kept that code. However, I could be easily convinced to delete this.

This revision was landed with ongoing or failed builds.Aug 27 2020, 6:06 AM
This revision was automatically updated to reflect the committed changes.