Page MenuHomePhabricator

[lldb-mi] Clean up and update a few MI commands.
ClosedPublic

Authored by apolyakov on Jun 10 2018, 7:53 AM.

Details

Summary

This patch updates a few MI commands using a new way of handling an errors in lldb-mi and removes unnecessary variables
m_lldbResult

Diff Detail

Repository
rL LLVM

Event Timeline

apolyakov created this revision.Jun 10 2018, 7:53 AM
apolyakov added a comment.EditedJun 10 2018, 7:55 AM

This patch uses changes from https://reviews.llvm.org/D47991. It allows to:

(gdb)
-file-exec-and-symbols "bash"
^done
(gdb)
=library-loaded,id="/bin/bash",target-name="/bin/bash",host-name="/bin/bash",symbols-loaded="0",loaded_addr="-",size="0"
-exec-next
^error,msg="this SBThread object is invalid"

instead of simple ^running message.

clayborg added inline comments.Jun 11 2018, 8:20 AM
tools/lldb-mi/MICmdCmdExec.cpp
373–377 ↗(On Diff #150651)

Maybe these four lines should be put into a utility function? Something like:

MIstatus ReturnMIStatus(lldb::SBError &error) {
  if (error.Success())
    return MIstatus::success;

  SetError(error.GetCString());
  return MIstatus::failure;
}

I am sure just about every MI command will end with something like this.

If you look at bool CMICmdCmdExecContinue::Execute(), you'll see that there are cases in which we need a flexible way to finish MI command(specific action in error case for example). We have a few options: not to add such an utility function, add and use it in simple situations where we just want to check on SBError status or we may create utility function with lambda functions in which user could specify actions he needs. What are your thoughts about this?

If you look at bool CMICmdCmdExecContinue::Execute(), you'll see that there are cases in which we need a flexible way to finish MI command(specific action in error case for example). We have a few options: not to add such an utility function, add and use it in simple situations where we just want to check on SBError status or we may create utility function with lambda functions in which user could specify actions he needs. What are your thoughts about this?

I am just trying to reduce duplicated code with my suggestion. Feel free to code normally when you are doing tricky things for returning in the middle of a function with special error cases.

apolyakov retitled this revision from [lldb-mi] Correct error processing in exec-next command. to [lldb-mi] Clean up and update a few MI commands..
apolyakov edited the summary of this revision. (Show Details)

I didn't committed patch https://reviews.llvm.org/D48295 yet, so I think if we change HandleSBError handlers from
std::function<bool/void()> to std::function<bool/boid(CMICmdBase *)> we'll be able to create anonymous namespace and declare there a function that takes a pointer to current command class(this pointer) instead if defining

auto successHandler = [this] {
    // CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
    if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
      const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
      this->SetError(CMIUtilString::Format(
          MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
          this->m_cmdData.strMiCmd.c_str(),
          rErrMsg.c_str()));
      return MIstatus::failure;
    }
    return MIstatus::success;

3 times in one file. It will reduce duplicating of code but I still think, is it worth it?

I didn't committed patch https://reviews.llvm.org/D48295 yet, so I think if we change HandleSBError handlers from
std::function<bool/void()> to std::function<bool/boid(CMICmdBase *)> we'll be able to create anonymous namespace and declare there a function that takes a pointer to current command class(this pointer) instead if defining

auto successHandler = [this] {
    // CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
    if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
      const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
      this->SetError(CMIUtilString::Format(
          MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
          this->m_cmdData.strMiCmd.c_str(),
          rErrMsg.c_str()));
      return MIstatus::failure;
    }
    return MIstatus::success;

3 times in one file. It will reduce duplicating of code but I still think, is it worth it?

Maybe add a command class that the 3 commands can inherit from and add a this code as a method that the other 3 commands can call? If this is specific to execution, it might consolidate the code nicely?

apolyakov added a comment.EditedJun 21 2018, 11:55 AM

I don't like creating a new class just for one feature. It looks like only 3 commands use that code, so maybe we should let it be as it is?

fine with leaving it as is if needed as well

Would be really nice to get review of this patch before this weekend. Thanks for your time, folks.

aprantl accepted this revision.Jun 22 2018, 2:55 PM
aprantl added inline comments.
tools/lldb-mi/MICmdCmdExec.cpp
137 ↗(On Diff #152351)

Is this a copy&paste error?

142 ↗(On Diff #152351)

Might want to factor this out into a HandleBoolReturnValue helper if it appears more than once.

This revision is now accepted and ready to land.Jun 22 2018, 2:55 PM
apolyakov added inline comments.Jun 23 2018, 3:35 AM
tools/lldb-mi/MICmdCmdExec.cpp
137 ↗(On Diff #152351)

It is, thanks.

142 ↗(On Diff #152351)

The good solution is to add anonymous namespace with a function bool HandleDriverStateChanging(bool status, CMICmdBase *cmd), but it's impossible since SetError and m_cmdData are protected members of CMICmdBase.

Another way is to add this function to CMICmdBase, but again there are only 3 commands that use that code. It seems unreasonable to do it for all commands.

apolyakov updated this revision to Diff 152589.Jun 23 2018, 3:46 AM

Removed accidentally added comment, added const qualifier to lambdas.

clayborg added inline comments.Jun 25 2018, 7:05 AM
tools/lldb-mi/MICmdCmdExec.cpp
139 ↗(On Diff #152589)

any reason for the "this->"? Remove it?

141 ↗(On Diff #152589)

any reason for the "this->"? Remove it?

244 ↗(On Diff #152589)

any reason for the "this->"? Remove it?

246 ↗(On Diff #152589)

any reason for the "this->"? Remove it?

apolyakov updated this revision to Diff 152694.Jun 25 2018, 8:03 AM

Removed unnecessary this->.

clayborg accepted this revision.Jun 25 2018, 8:10 AM
This revision was automatically updated to reflect the committed changes.