This is an archive of the discontinued LLVM Phabricator instance.

[lldb/DWARF] Switch to llvm location list parser
ClosedPublic

Authored by labath on Dec 4 2019, 4:28 AM.

Details

Summary

This patch deletes the lldb location list parser and teaches the
DWARFExpression class to use the parser in llvm instead. I have
centralized all the places doing the parsing into a single
GetLocationExpression function.

In theory the the actual location list parsing should be covered by llvm
tests, and this glue code by our existing location list tests, but since
we don't have that many location list tests, I've tried to extend the
coverage a bit by adding some explicit dwarf5 loclist handling and a
test of the dumping code.

For DWARF4 location lists this should be NFC (modulo small differences
in error handling which should only show up on invalid inputs). In case
of DWARF5, this fixes various missing bits of functionality, most
notably, the lack of support for DW_LLE_offset_pair.

Event Timeline

labath created this revision.Dec 4 2019, 4:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 4:28 AM
dblaikie added inline comments.Dec 4 2019, 5:24 AM
lldb/source/Expression/DWARFExpression.cpp
92

I think usually the unreachable comment that's used in this sort of case is "Invalid LocationListFormat!" or similar, to give a bit more context about the situation.

637

I'd probably write this as "... != None" rather than "bool(...)" - would make it clearer what the return type of GetLocationExpression is/what conversion is being done here.

2830

Does LLDB_LOG_ERROR stop the program? If not, then the next line ("if (loc->Range)" will invoke UB when it tries to dereference the "loc" when it is unset.

labath marked an inline comment as done.Dec 4 2019, 5:43 AM
labath added inline comments.
lldb/source/Expression/DWARFExpression.cpp
2830

It doesn't. You're right, there should be a return here, just let me see if I can tickle this into exploding.

labath updated this revision to Diff 232106.Dec 4 2019, 6:08 AM
labath marked 4 inline comments as done.

Feedback @dblaikie.

lldb/source/Expression/DWARFExpression.cpp
2830

It turns out triggering this was pretty hard, because the main source of these errors is a failed indirect address resolution, but our callback never returned any errors. I've now fixed it to return errors and included an additional test.

dblaikie added inline comments.Dec 4 2019, 8:46 AM
lldb/source/Expression/DWARFExpression.cpp
2830

Looks great!

That's about all the thoughts I have on the patch - will leave the rest to LLDB developers.

JDevlieghere added inline comments.Dec 4 2019, 9:20 AM
lldb/include/lldb/Expression/DWARFExpression.h
260

Should this be part of the DWARFExpression API? It feels like a static function might be sufficient. Maybe it could take a range directly?

lldb/source/Expression/DWARFExpression.cpp
53

Why the unchecked?

158
llvm::MCRegisterInfo *MRI = abi ? &abi->GetMCRegisterInfo() : nullptr;
2799

Should this be a doxygen comment on the function itself?

Nice!

lldb/source/Expression/DWARFExpression.cpp
57

Doxygen comment?

132

Doxygen comment: what is this used for?

labath updated this revision to Diff 232319.Dec 5 2019, 5:09 AM
labath marked 10 inline comments as done.
  • "inline" RelocateLowHighPC
  • add some comments
labath added inline comments.Dec 5 2019, 5:10 AM
lldb/include/lldb/Expression/DWARFExpression.h
260

It's a private member function, so it's not really a part of any API. However, given that now it's called from only a single place, it's not actually useful. It served a purpose in the preceding patch (I'm going to need a stamp on that too BTW), since there it had like five callers, but now that this patch centralizes them, I can just inline it.

lldb/source/Expression/DWARFExpression.cpp
53

Because I've already checked that the offset is valid (that's the difference between the checked and unchecked versions, though I'm not really sure if we need them).

57

I've added /something/, though I think the purpose is mostly obvious here.

Btw, this part is going to get refactored further, since the current design doesn't really permit mixing DWARF 4&5 location lists within a single object file.

JDevlieghere accepted this revision.Dec 5 2019, 9:09 AM
This revision is now accepted and ready to land.Dec 5 2019, 9:09 AM
This revision was automatically updated to reflect the committed changes.