This is an archive of the discontinued LLVM Phabricator instance.

[lldb-mi] Fix implementation for a few mi commands
ClosedPublic

Authored by aetf on Sep 18 2016, 2:17 PM.

Details

Summary

Some of the mi commands implemented in lldb-mi are incomplete/not confirming to the spec.

  • gdb-show and gdb-set doesn't support getting/setting disassembly-flavor
  • environment-cd should also change the working directory for inferior
  • debugger CLI output should be printed as console-stream-output record, rather than being dumped directly

to stdout

  • target-select should provide inner error message in mi response

Related bug report:

Diff Detail

Repository
rL LLVM

Event Timeline

aetf updated this revision to Diff 71760.Sep 18 2016, 2:17 PM
aetf retitled this revision from to [lldb-mi] Fix implementation for a few mi commands.
aetf updated this object.
aetf added a reviewer: ki.stfu.
aetf added a subscriber: lldb-commits.
aetf set the repository for this revision to rL LLVM.Sep 18 2016, 2:17 PM
aetf added a project: Restricted Project.
aetf updated this object.
ki.stfu requested changes to this revision.Sep 18 2016, 8:57 PM
ki.stfu edited edge metadata.

Hi! Please add tests for commands that you fixed. I'll take a look later this week.

This revision now requires changes to proceed.Sep 18 2016, 8:57 PM
aetf added a comment.Sep 19 2016, 9:43 AM

Okay, tests should go to packages/Python/lldbsuite/test/tools/lldb-mi, right? I need to get familiar with them first.

abidh added a subscriber: abidh.Sep 21 2016, 5:57 AM
abidh added a comment.Sep 21 2016, 6:41 AM

Changes looks mostly Ok to me apart from some comments. Please address them and add testcases as mentioned by ilia. Also try to do one review for one fix. This review is for 3 fixes. When the changes are approved, please commit them in 3 separate commits (one per fix).

tools/lldb-mi/MICmdCmdGdbSet.cpp
421 ↗(On Diff #71760)

You are not checking if SetInternalVariable returned an error which can happen in this case if the flavor name was not one of "intel", "att" or "default".

tools/lldb-mi/MICmdCmdMiscellanous.cpp
515 ↗(On Diff #71760)

It is not really an OutofBand record but rather an output of the command. Why not simple add prepend an ~

tools/lldb-mi/MICmdCmdTarget.cpp
126 ↗(On Diff #71760)

This does not seem related to any bug fix.

aetf updated this revision to Diff 82654.Dec 29 2016, 1:15 AM
aetf edited edge metadata.
aetf marked an inline comment as done.

Add unit tests, add error checking for SetInternalVariable

aetf added a comment.Dec 29 2016, 1:16 AM

Hi, sorry for the long delay. It has been a busy semester.

Added unit tests are

  • MiGdbSetShowTestCase.test_lldbmi_gdb_set_disassembly_flavor for disassembly flavor settings. Note this one doesn't pass currently due to https://llvm.org/bugs/show_bug.cgi?id=31485
  • MiEnvironmentCdTestCase.test_lldbmi_environment_cd for -environment-cd command

Extended test case:

  • MiSyntaxTestCase.test_lldbmi_output_grammar to include a command generating console stream records

Fixed test case:

  • MiExecTestCase.test_lldbmi_exec_arguments_set which breaks because of the console stream records

I tested against latest trunk version (290647), and all tests passed with 5 expected failures and 3 unexpected success:

  • MiGdbSetShowTestCase.test_lldbmi_gdb_set_target_async_off
  • MiInterpreterExecTestCase.test_lldbmi_settings_set_target_run_args_after
  • MiSyntaxTestCase.test_lldbmi_process_output

I don't have the commit access, but I can create separate RRs if necessary. (should the target select error fix go to it's own RR?)

tools/lldb-mi/MICmdCmdMiscellanous.cpp
515 ↗(On Diff #71760)

The output of the command should be a Stream record which is an OutofBand record according to the spec [1]. I agree it's no more than prepending '~' and quoting the string. But why not just do what the spec says? ;-)

[1] https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Stream-Records.html#GDB_002fMI-Stream-Records

tools/lldb-mi/MICmdCmdTarget.cpp
126 ↗(On Diff #71760)

I didn't create a bug report for this since it's not a big deal. But it's rather annoying when debugging because the actual error description never gets to the output.

abidh accepted this revision.Jan 3 2017, 2:41 AM
abidh edited edge metadata.

Looks good.

packages/Python/lldbsuite/test/tools/lldb-mi/main.cpp
22 ↗(On Diff #82654)

This declaration looks redundant.

tools/lldb-mi/MICmdCmdMiscellanous.cpp
515 ↗(On Diff #71760)

ok.

aetf marked 2 inline comments as done.Jan 4 2017, 9:33 AM

What do I do next? Could you help me commit and push this? since I don't have write access.

packages/Python/lldbsuite/test/tools/lldb-mi/main.cpp
22 ↗(On Diff #82654)

It's used in TestMiGdbSetShow.test_lldbmi_gdb_set_ouptut_radix.

abidh added a comment.Jan 4 2017, 9:43 AM

What do I do next? Could you help me commit and push this? since I don't have write access.

I will commit it for you.

This revision was automatically updated to reflect the committed changes.