This is an archive of the discontinued LLVM Phabricator instance.

Set "success" status correctly
Needs ReviewPublic

Authored by jafl on Apr 21 2017, 11:52 AM.

Details

Reviewers
clayborg
labath
Summary

In CommandObjectRegisterRead.DoExecute(), set the status to "success" when appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

jafl created this revision.Apr 21 2017, 11:52 AM
labath added a subscriber: labath.Apr 24 2017, 3:43 AM

Thanks for the patch. Could you please also add an appropriate test for it? Doing something similar to what packages/Python/lldbsuite/test/functionalities/frame_var/TestFrameVar.py does should be the easiest way to test this.

It can make sense to add

assert(eReturnStatusStarted != result);

after cmd_obj->Execute() invocation at CommandInterpreter::HandleCommand, do you think?

that sounds like an excellent idea, as it will check all executed commands, and not the ones we've remembered checking. It should probably be an lldbassert though. (And we'd need to check that the existing tests still pass after that.)

jafl added a comment.May 24 2017, 10:14 AM

Thanks for the suggestions. I will get around to this - just swamped right now!

Yes: lldbassert would be fine for that since those get compiled out during release. Patch looks fine. If we already have a test that would trigger the new "lldbassert" you will add, then no need for a special test for this, else we need a test that triggers this.

clayborg added a reviewer: labath.
jafl added a comment.Aug 28 2017, 11:14 AM

I will try to ensure that the lldbassert gets tested, but right now, none of the tests work on my machine. I emailed lldb-dev for help.

jafl updated this revision to Diff 112922.Aug 28 2017, 11:18 AM

Added lldbassert

clayborg edited edge metadata.Aug 28 2017, 11:37 AM

This is sure to trigger things in the test suite. We will need to ensure a few things:

  • test suite runs cleanly in debug mode after the lldbassert is added
  • without changes to CommandObjectRegister.cpp that the lldbassert is triggered, and if not, add a test
jafl added a comment.Aug 28 2017, 11:38 AM

Absolutely!

labath resigned from this revision.May 8 2018, 8:54 AM