Page MenuHomePhabricator

[lldb] Replace the LEB128 decoding logic in LLDB's DataExtractor with calls to LLVM's LEB128 implementation
ClosedPublic

Authored by teemperor on Jun 9 2020, 2:41 AM.

Diff Detail

Event Timeline

teemperor updated this revision to Diff 269461.Jun 9 2020, 2:41 AM
teemperor created this revision.
  • Fixed include ordering.
labath added inline comments.Jun 9 2020, 2:45 AM
lldb/source/Utility/DataExtractor.cpp
884

Ummm.. why don't you just pass m_end here? The last argument is the end of the buffer, not the end of the number...

teemperor marked an inline comment as done.Jun 9 2020, 3:36 AM
teemperor added inline comments.
lldb/source/Utility/DataExtractor.cpp
884

From what I can see decodeSLEB128 doesn't have any detection logic for ints > 64bits, so it would just keep reading data until it finds something that looks like the terminating byte (or the end of the buffer) in case of malformed input. But if we explicitly limit the buffer to 10 bytes then it will stop and set a reasonable error message (even though we don't use that yet in LLDB). I did the same for decodeULEB128 just for the sake of symmetry (as that function curiously figures out this error case on its own).

I'm fine with just passing m_end here though (that would make this patch more NFC as we currently also don't handle malformed input).

labath added inline comments.Jun 9 2020, 7:33 AM
lldb/source/Utility/DataExtractor.cpp
884

If we're going to be using that function, I think we ought to rely on it to determine what constitutes a malformed LEB128. Also, the current version has a bug in that it does not check whether src + max_int64_size is still inside the data extractor range.

teemperor updated this revision to Diff 269543.Jun 9 2020, 7:58 AM
  • Pass m_end as buffer end instead.
teemperor marked an inline comment as done.Jun 9 2020, 8:00 AM
teemperor added inline comments.
lldb/source/Utility/DataExtractor.cpp
884

Yeah, I think you're right. Updated the patch, thanks!

Looks good

lldb/source/Utility/DataExtractor.cpp
911

Should we hoist this into llvm as well (as a separate patch). It seems like at least lld/ELF/EhFrame.cpp is using the same functionality.

labath accepted this revision.Jun 10 2020, 3:48 AM
labath added inline comments.
lldb/source/Utility/DataExtractor.cpp
911

TBH, I'm tempted to just delete it. I doubt that "skipping" an LEB128 is going to be noticeably faster than "reading" it, and llvm::DataExtractor has no equivalent function...

This revision is now accepted and ready to land.Jun 10 2020, 3:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 7:38 AM
JDevlieghere added inline comments.Jun 10 2020, 9:03 AM
lldb/source/Utility/DataExtractor.cpp
911

Even better :-)