Page MenuHomePhabricator

Implement DW_OP_convert
ClosedPublic

Authored by aprantl on Sep 9 2019, 1:32 PM.

Details

Summary

This patch adds basic support for DW_OP_convert[1] for integer types. Recent versions of LLVM's optimizer may insert this opcode into DWARF expressions. DW_OP_convert is effectively a type cast operation that takes a reference to a base type DIE (or zero) and then casts the value at the top of the DWARF stack to that type. Internally this works by changing the bit size of the APInt that is used as backing storage for LLDB's DWARF stack.

I managed to write a unit test for this by implementing a mock YAML object file / module that takes debug info sections in yaml2obj format.

[1] Typed DWARF stack. http://www.dwarfstd.org/ShowIssue.php?issue=140425.1

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Sep 9 2019, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 1:32 PM
jasonmolenda accepted this revision.Sep 9 2019, 3:26 PM

LGTM, nice job with the test case, I see myself copying that in the future. :)

lldb/source/Utility/Scalar.cpp
423 ↗(On Diff #219422)

What about an ILP32 target like arm64_32, with lldb running on an LP64 system - are we mixing target & host type sizes here? I'm not sure if Scalar's types (e_slong etc) are in host or target terms. I mean this code might as well be if bit_size <= 32 return Scalar::e_Signed32BitType, but I wanted to check in because we're using host type sizes here.

This revision is now accepted and ready to land.Sep 9 2019, 3:26 PM
aprantl added a reviewer: vsk.Sep 9 2019, 3:57 PM
aprantl marked an inline comment as done.
aprantl added inline comments.
lldb/source/Utility/Scalar.cpp
423 ↗(On Diff #219422)

As the comment on line 420 above says:
// Scalar types are always host types, hence the sizeof().

aprantl marked an inline comment as done.Sep 9 2019, 4:11 PM
aprantl added inline comments.
lldb/source/Utility/Scalar.cpp
423 ↗(On Diff #219422)

Note that I think the decision of making Scalar types host types very questionable and it would make much more sense for it to be target types, but the way the Scalar constructors are implemented they are definitely host types.

JDevlieghere added inline comments.
lldb/include/lldb/Utility/Scalar.h
107 ↗(On Diff #219422)

How about GetTypeForBitSize?

lldb/source/Expression/DWARFExpression.cpp
2571 ↗(On Diff #219422)

Can we wrap this in a lambda?

lldb/source/Utility/Scalar.cpp
442 ↗(On Diff #219422)

llvm::unreachable?

aprantl marked an inline comment as done.Sep 9 2019, 4:33 PM
aprantl added inline comments.
lldb/source/Expression/DWARFExpression.cpp
2571 ↗(On Diff #219422)

Which part specifically do you mean?

JDevlieghere added inline comments.Sep 9 2019, 4:45 PM
lldb/source/Expression/DWARFExpression.cpp
2571 ↗(On Diff #219422)
auto SetErrorString = [&](llvm::StringRef msg) { if (error_ptr) error_ptr->SetErrorString(msg); })

We could deduplicate even more code by using a macro that also covers the check around it and does the return, but I'm not sure if that's really worth it.

vsk added a comment.Sep 9 2019, 5:45 PM

Looks good overall, especially the testing.

lldb/include/lldb/Utility/Scalar.h
107 ↗(On Diff #219422)

nit: bool signed ?

lldb/source/Expression/DWARFExpression.cpp
2571 ↗(On Diff #219422)

Oh, like LLDB_LOG? Seems like a nice idea. I think I need this for D67376, so I might do it if no one else has started already. It'd be great to use that here.

aprantl marked an inline comment as done.Sep 10 2019, 8:55 AM
aprantl added inline comments.
lldb/include/lldb/Utility/Scalar.h
107 ↗(On Diff #219422)

that name conflicts with the actual type signed.

aprantl marked 5 inline comments as done.Sep 10 2019, 9:04 AM
aprantl added inline comments.
lldb/source/Expression/DWARFExpression.cpp
2571 ↗(On Diff #219422)

I'll leave this as an NFC refactoring for later since it will affect code outside of this patch.

Closed by commit rL371532: Implement DW_OP_convert (authored by adrian, committed by ). · Explain WhySep 10 2019, 9:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 9:19 AM

The yaml object file is pretty impressive, but I am wondering if it is really necessary. Have you looked if it would be possible to reuse the existing TestFile::fromYaml functionality? I think it should be possible to reuse that by just inserting some MachO yaml blurbs around the dwarf yaml (elf yaml2obj doesn't support embedding dwarf yaml, but that's just because nobody enabled that yet).