[lldb, lldb-mi] Re-implement MI -exec-continue command.

[polyakov.alex]

Summary

Now -exec-continue command uses SB API to resume target's process.

[clayborg]

So nice to get rid of these HandleCommand hacks!

[polyakov.alex]

Updated because of commited -exec-next and -exec-step commands.

[clayborg]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

An error can claim to fail but not have an error string set. It might be nice to have a helper function that checks to make sure there is an error string on error cases and if there is no error string when error.Success() is false or error.Fail() is true, then set the error string to "unknown error".

[polyakov.alex]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

This functionality might be useful in all lldb-mi commands. So do you know where to place this function? Maybe inside SBError class?

[clayborg]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

I would put it somewhere in lldb-mi in a static function that is something like:

static void SetErrorString(lldb::SBError &error, T &lldbResult) {
  const char *error_cstr = error.GetCString();
  if (error_cstr)
    lldbResult.SetError(error.GetCString());
  else
    lldbResult.SetError("unknown error");
}

Where the T is the type of m_lldbResult.

[clayborg]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

Actually, are we using m_lldbResult now? I didn't realize its type was lldb::SBCommandReturnObject. That was only needed if we were calling:

rSessionInfo.GetDebugger().GetCommandInterpreter().HandleCommand(pCmd, m_lldbResult);

So we can get rid of all "lldb::SBCommandReturnObject m_lldbResult" member variables in any lldb-mi functions where we switch to using the API.

[clayborg]

Also need to remove m_lldbResult from the CMICmdCmdExecContinue class.

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

Since we can get rid of m_lldbResult, this should be:

} else return MIstatus::failure;

[polyakov.alex]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

It should be:

else {
  SetError(CMIUtilString::Format(..., error.GetCString()));
  return MIstatus::failure;
}

So, we anyway need a C-style error string.

[labath]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

AFAICT, SBError::GetCString calls Status::AsCString which has a default argument const char *default_error_str = "unknown error". So it sounds like this issue is already taken care of down below. If we are still getting null strings for failed SBErrors, maybe we need to fix something else instead of adding another layer here.

[apolyakov]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

SBError::GetCString potentially may return NULL:

const char *SBError::GetCString() const {
  if (m_opaque_ap.get())
    return m_opaque_ap->AsCString();
  return NULL;
}

[labath]

tools/lldb-mi/MICmdCmdExec.cpp

248 ↗

(On Diff #150391)

Yes, but that can happen only in case SBError::Fail() is false, which means you probably shouldn't be calling GetCString in the first place.

[polyakov.alex]

Removed unnecessary m_lldbResult attribute.

lldb/trunk/lit/tools/lldb-mi/exec/exec-continue.test

+# XFAIL: windows
+# -> llvm.org/pr24452
+#
+# RUN: %cc -o %t %p/inputs/main.c -g
+# RUN: %lldbmi %t < %s | FileCheck %s
+
+# Test lldb-mi -exec-continue command.
+
+# Check that we have a valid target created via '%lldbmi %t'.
+# CHECK: ^done
+
+-break-insert main
+# CHECK: ^done,bkpt={number="1"
+
+-exec-run
+# CHECK: ^running
+# CHECK: *stopped,reason="breakpoint-hit"
+
+-exec-continue
+# CHECK: ^running

lldb/trunk/tools/lldb-mi/MICmdCmdExec.h

 public:
   // From CMICmdInvoker::ICmd
   bool Execute() override;
   bool Acknowledge() override;
   // From CMICmnBase
   /* dtor */ ~CMICmdCmdExecContinue() override;
 
   // Attributes:
-private:
-  lldb::SBCommandReturnObject m_lldbResult;
 };
 
 //++
 //============================================================================
 // Details: MI command class. MI commands derived from the command base class.
 //          *this class implements MI command "exec-next".
 //--
 class CMICmdCmdExecNext : public CMICmdBase {

lldb/trunk/tools/lldb-mi/MICmdCmdExec.cpp

 //          here.
 // Type:    Overridden.
 // Args:    None.
 // Return:  MIstatus::success - Functional succeeded.
 //          MIstatus::failure - Functional failed.
 // Throws:  None.
 //--
 bool CMICmdCmdExecContinue::Execute() {
-  const char *pCmd = "continue";
-  CMICmnLLDBDebugSessionInfo &rSessionInfo(
-      CMICmnLLDBDebugSessionInfo::Instance());
-  const lldb::ReturnStatus rtn =
-      rSessionInfo.GetDebugger().GetCommandInterpreter().HandleCommand(
-          pCmd, m_lldbResult);
-  MIunused(rtn);
+  lldb::SBError error =
+      CMICmnLLDBDebugSessionInfo::Instance().GetProcess().Continue();
  
-  if (m_lldbResult.GetErrorSize() == 0) {
+  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;
     }
-  } else {
-    // ToDo: Re-evaluate if this is required when application near finished as
-    // this is parsing LLDB error message
-    // which seems a hack and is code brittle
-    const char *pLldbErr = m_lldbResult.GetError();
-    const CMIUtilString strLldbMsg(CMIUtilString(pLldbErr).StripCREndOfLine());
-    if (strLldbMsg == "error: Process must be launched.") {
-      CMIDriver::Instance().SetExitApplicationFlag(true);
-    }
+    return MIstatus::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 CMICmdCmdExecContinue::Acknowledge() {
-  if (m_lldbResult.GetErrorSize() > 0) {
-    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;
-  }
-
   return MIstatus::success;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: Required by the CMICmdFactory when registering *this command. The
 // factory
 //          calls this function to create an instance of *this command.