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

[apolyakov]

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

[apolyakov]

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]

tools/lldb-mi/MICmdCmdExec.cpp

373–377

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.

[apolyakov]

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?

[clayborg]

In D47992#1134120, @apolyakov wrote:

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]

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?

[clayborg]

In D47992#1139691, @apolyakov wrote:

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]

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?

[clayborg]

fine with leaving it as is if needed as well

[apolyakov]

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

[aprantl]

tools/lldb-mi/MICmdCmdExec.cpp

144

Is this a copy&paste error?

149

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

[apolyakov]

tools/lldb-mi/MICmdCmdExec.cpp

144

It is, thanks.

149

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]

Removed accidentally added comment, added const qualifier to lambdas.

[clayborg]

tools/lldb-mi/MICmdCmdExec.cpp

146

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

148

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

245

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

247

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

[apolyakov]

Removed unnecessary this->.

tools/lldb-mi/MICmdCmdExec.h

public:
   bool Execute() override;
   bool Acknowledge() override;
   bool ParseArgs() override;
   // From CMICmnBase
   /* dtor */ ~CMICmdCmdExecNext() override;
 
   // Attributes:
 private:
-  lldb::SBCommandReturnObject m_lldbResult;
   const CMIUtilString m_constStrArgNumber; // Not specified in MI spec but
                                            // Eclipse gives this option
 };
 
 //++
 //============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "exec-step".

tools/lldb-mi/MICmdCmdExec.cpp

bool CMICmdCmdExecRun::Execute() {
   if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
     const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
     SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
                                    m_cmdData.strMiCmd.c_str(),
                                    rErrMsg.c_str()));
     return MIstatus::failure;
   }
   return MIstatus::success;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command prepares a MI Record
 // Result
 //          for the work carried out in the Execute().
 //          Called only if Execute() set status as successful on completion.
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
lldb::SBError error =
       CMICmnLLDBDebugSessionInfo::Instance().GetProcess().Continue();
  
   if (error.Success()) {
     // CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
     if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
       const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
       SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
                                      m_cmdData.strMiCmd.c_str(),
                                      rErrMsg.c_str()));
       return MIstatus::failure;
     }
     return MIstatus::success;
   }
 
   SetError(error.GetCString());
   return MIstatus::failure;
 }
 
 //++
SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_THREAD_INVALID),
                                    m_cmdData.strMiCmd.c_str(),
                                    m_constStrArgThread.c_str()));
     return MIstatus::failure;
   }
 
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
       CMICmnLLDBDebugSessionInfo::Instance());
 
+  lldb::SBError error;
   if (nThreadId != UINT64_MAX) {
     lldb::SBThread sbThread = rSessionInfo.GetProcess().GetThreadByIndexID(nThreadId);
     if (!sbThread.IsValid()) {
       SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_THREAD_INVALID),
                                      m_cmdData.strMiCmd.c_str(),
                                      m_constStrArgThread.c_str()));
       return MIstatus::failure;
     }
-    sbThread.StepOver();
-  } else rSessionInfo.GetProcess().GetSelectedThread().StepOver();
+    sbThread.StepOver(error);
+  } else rSessionInfo.GetProcess().GetSelectedThread().StepOver(error);
 
+  if (error.Success())
     return MIstatus::success;
+
+  SetError(error.GetCString());
+  return MIstatus::failure;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: The invoker requires this function. The command prepares a MI Record
 // Result
 //          for the work carried out in the Execute().
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool CMICmdCmdExecNext::Acknowledge() {
-  if (m_lldbResult.GetErrorSize() > 0) {
-    const char *pLldbErr = m_lldbResult.GetError();
-    MIunused(pLldbErr);
-    const CMICmnMIValueConst miValueConst(m_lldbResult.GetError());
-    const CMICmnMIValueResult miValueResult("message", miValueConst);
-    const CMICmnMIResultRecord miRecordResult(
-        m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Error,
-        miValueResult);
-    m_miResultRecord = miRecordResult;
-  } else {
-    const CMICmnMIResultRecord miRecordResult(
-        m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Running);
-    m_miResultRecord = miRecordResult;
-  }
-
+  m_miResultRecord = CMICmnMIResultRecord(
   return MIstatus::success;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: Required by the CMICmdFactory when registering *this command. The
 // factory
 //          calls this function to create an instance of *this command.