This is an archive of the discontinued LLVM Phabricator instance.

Teach DWARFExpression about DWARF 4+ Location Descriptions
ClosedPublic

Authored by aprantl on Mar 19 2021, 5:38 PM.

Details

Summary

DWARFExpression implements the DWARF2 expression model that left ambiguity on whether the result of an expression was a value or an address. This patch implements the DWARF location description model introduces in DWARF 4 and sets the result Value's kind accordingly, if the expression comes from a DWARF v4+ compile unit. The nomenclature is taken from DWARF 5, chapter 2.6 "Location Descriptions".

Diff Detail

Event Timeline

aprantl requested review of this revision.Mar 19 2021, 5:38 PM
aprantl created this revision.

(idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather than away from?)

jasonmolenda accepted this revision.Mar 19 2021, 11:24 PM

This fits with my reading of the DWARF5 doc; LGTM.

This revision is now accepted and ready to land.Mar 19 2021, 11:24 PM

Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I follow this right, then the final value type will be determined by the last operand. That sounds like it could be right for regular dwarf expressions, but I'm not sure about those with pieces. The classification function will mark those as "memory", and that doesn't sound right...

lldb/unittests/Expression/DWARFExpressionTest.cpp
360–361

This comment doesn't seem right.

JDevlieghere added inline comments.Mar 22 2021, 8:39 AM
lldb/source/Expression/DWARFExpression.cpp
1000

That also makes it more explicit that this is updating the dwarf4_location_description_kind

1029

Could you return the kind and do LocationDescriptionKind dwarf4_location_description_kind = classifyLocationDescription? That way you can make it a static instead of a lambda in a function that's already a hundreds of lines long.

2655

lldb_assert?

@dblaikie wrote:

(idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather than away from?)

There's definitely an opportunity to refactor the main loop of Evaluate() to use llvm::DWARFExpression::iterator. I would say The impact of this patch towards that goal is neutral. The iterator (and all of libDebugInfo) doesn't care about the semantics of DWARF expressions.

lldb/source/Expression/DWARFExpression.cpp
2634

@labath wrote:

Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I follow this right, then the final value type will be determined by the last operand. That sounds like it could be right for regular dwarf expressions, but I'm not sure about those with pieces. The classification function will mark those as "memory", and that doesn't sound right...

DW_OP_pieces are handled in this block.

For your immediate question, this means that this patch is safe. However, it also highlights that this patch isn't fixing the same bug in a composite location description. I'll address this in my next revision of the patch.

aprantl updated this revision to Diff 332493.Mar 22 2021, 6:36 PM

Updated the patch to compute the classification on the fly and take composition operators into account.

This revision was landed with ongoing or failed builds.Mar 23 2021, 10:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 10:30 AM