This is an archive of the discontinued LLVM Phabricator instance.

Fix LLDB-MI -data-read-memory-bytes command to comply with GDB/MI spec
ClosedPublic

Authored by enlight on May 4 2015, 5:56 AM.

Details

Summary
  • The address argument can now be an expression (e.g. &array), it's no longer restricted to being just a number literal.
  • The -o (offset) option is now properly handled, not just silently ignored.
  • The --thread option is now properly handled.
  • A new --frame option has been added for consistency with GDB.
  • Added a new test to verify old and new functionality.

Diff Detail

Repository
rL LLVM

Event Timeline

enlight updated this revision to Diff 24877.May 4 2015, 5:56 AM
enlight retitled this revision from to Fix LLDB-MI -data-read-memory-bytes command to comply with GDB/MI spec.
enlight updated this object.
enlight edited the test plan for this revision. (Show Details)
enlight set the repository for this revision to rL LLVM.
enlight added a subscriber: Unknown Object (MLST).
ki.stfu requested changes to this revision.May 4 2015, 10:00 PM
ki.stfu edited edge metadata.

Great job! Looks good apart from a 1 inline comment.

Btw, please update tools/lldb-mi/MIExtensions.txt if your implementation has any non-standard functionality.

tools/lldb-mi/MICmdCmdData.cpp
690 ↗(On Diff #24877)

Looks like it isn't needed. Remove it please.

This revision now requires changes to proceed.May 4 2015, 10:00 PM
enlight updated this revision to Diff 24936.May 5 2015, 12:27 AM
enlight edited edge metadata.

Removed the line requested, added a full description of the command to MIExtensions.txt.

abidh accepted this revision.May 5 2015, 2:37 AM
abidh edited edge metadata.
abidh added inline comments.
tools/lldb-mi/MICmdCmdData.cpp
730 ↗(On Diff #24936)

Not an objection on your patch but this "offset" field seems redundant to me as it is always 0. GDB seems to have the same behavior too.

All tests pass on OS X.

enlight added inline comments.May 6 2015, 8:50 PM
tools/lldb-mi/MICmdCmdData.cpp
730 ↗(On Diff #24936)

GDB sets the block offset to something other than zero in cases where the specified memory range has inaccessible regions, in that case it may return one or more blocks where begin != (address + byte-offset) and therefore the block offset will be (begin - (address + byte-offset)) > 0, where address and byte-offset correspond to the arguments passed into -data-read-memory-bytes. The way GDB deals with inaccessible regions is described in the MI spec, but the current implementation in LLDB-MI doesn't do any of that. In LLDB-MI the command only succeeds if the entire memory range is read successfully, if any of it is inaccessible the command simply returns an error.

I suppose the title I chose for this patch is not entirely accurate, my changes bring us closer to complying with the MI spec, but there's still some way to go :)

ki.stfu accepted this revision.May 6 2015, 10:19 PM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.May 6 2015, 10:19 PM
brucem added a subscriber: brucem.May 6 2015, 10:20 PM

I can land this for Vadim. (Doing it now unless I hear otherwise.)

This revision was automatically updated to reflect the committed changes.