[lldb-mi] Re-implement MI -exec-step command.

[polyakov.alex]

Summary

Now -exec-step uses SB API instead of HandleCommand hack.

[polyakov.alex]

I'm a little bit confused about situation with thread id. For example:

(gdb)
thread list
~"Process 31642 stopped\n* thread #1: tid = 31642, 0x000000000041eed0 bash`main, name = 'bash', stop reason = breakpoint 1.1\n"
^done

What we should use as a thread ID? Thread number or tid? The main problem is that before my changes exec-like commands used thread number, but SBThread uses tid, as I know.

[aprantl]

Did the old implementation come with a testcase? Perhaps I'm misunderstanding the question, but it would probably be best to preserve the old behavior.

tools/lldb-mi/MICmdCmdExec.cpp

515 ↗

(On Diff #150168)

Usually we prefer early exits in LLVM code (https://www.llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)

if (!sbThread.IsValid)
  return failure;
sbThread.StepInto();

[labath]

I don't know if there is any spec about what the gdb-mi protocol should use for thread identification, but I think the most important part is to be consistent. If you use indexes in one place and tid's in another you'll confuse the hell out of clients trying to talk to lldb-mi.

[polyakov.alex]

In D47838#1124730, @labath wrote:

I don't know if there is any spec about what the gdb-mi protocol should use for thread identification, but I think the most important part is to be consistent. If you use indexes in one place and tid's in another you'll confuse the hell out of clients trying to talk to lldb-mi.

Due to https://sourceware.org/gdb/onlinedocs/gdb/Threads.html, gdb has its own thread numbers for each thread of an inferior.
So we should do the same, I guess.

[polyakov.alex]

Changes for using thread ids in a gdb way. Also added case of specified thread into test case.

[polyakov.alex]

Removed unnecessary m_lldbResult attribute, updated python test to successfully process new error messages in exec-step command.

[clayborg]

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

505–509

Utility funciton for these last four line? I mentioned it in another review:

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

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

524

How does this code handle the error case for stepping now?

[polyakov.alex]

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

524

If there is an error in lldb-mi command execution, it'll be detected in Execute function:

if (error.Success())
  return MIstatus::sucess;

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

If Execute function returns MIstatus::failure, then Acknowledge function won't be called.

lldb/trunk/lit/tools/lldb-mi/exec/exec-step.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-step 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-step --thread 0
+# Check that exec-step can process the case of invalid thread ID.
+# CHECK: ^error,msg="Command 'exec-step'. Thread ID invalid"
+
+-exec-step --thread 1
+# CHECK: ^running
+# CHECK: *stopped,reason="end-stepping-range"
+
+-exec-step
+# Check that exec-step can step-in in a selected thread.
+# CHECK: ^running
+# CHECK: *stopped,reason="end-stepping-range"

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/control/TestMiExec.py

def test_lldbmi_exec_step(self):
         # s_MyFunction (and that both --thread and --frame are optional)
         self.runCmd("-exec-step")
         self.expect("\^running")
         self.expect(
             "\*stopped,reason=\"end-stepping-range\".+?func=\"g_MyFunction.*?\"")
 
         # Test that an invalid --thread is handled
         self.runCmd("-exec-step --thread 0")
-        self.expect("\^error,message=\"error: Thread index 0 is out of range")
+        self.expect("\^error,msg=\"Command 'exec-step'. Thread ID invalid")
         self.runCmd("-exec-step --thread 10")
-        self.expect("\^error,message=\"error: Thread index 10 is out of range")
+        self.expect("\^error,msg=\"Command 'exec-step'. Thread ID invalid")
 
         # Test that an invalid --frame is handled
         # FIXME: no error is returned
         self.runCmd("-exec-step --frame 10")
         #self.expect("\^error: Frame index 10 is out of range")
 
     @skipIfRemote   # We do not currently support remote debugging via the MI.
     @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows

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

public:
   bool Execute() override;
   bool Acknowledge() override;
   bool ParseArgs() override;
   // From CMICmnBase
   /* dtor */ ~CMICmdCmdExecStep() 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-next-instruction".

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

if (pArgThread->GetFound() &&
     SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_OPTION_NOT_FOUND),
                                    m_cmdData.strMiCmd.c_str(),
                                    m_constStrArgThread.c_str()));
     return MIstatus::failure;
   }
 
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
       CMICmnLLDBDebugSessionInfo::Instance());
-  lldb::SBDebugger &rDebugger = rSessionInfo.GetDebugger();
-  CMIUtilString strCmd("thread step-in");
-  if (nThreadId != UINT64_MAX)
-    strCmd += CMIUtilString::Format(" %llu", nThreadId);
-  rDebugger.GetCommandInterpreter().HandleCommand(strCmd.c_str(), m_lldbResult,
-                                                  false);
 
+  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.StepInto(nullptr, LLDB_INVALID_LINE_NUMBER, error);
+  } else rSessionInfo.GetProcess().GetSelectedThread().StepInto(
+             nullptr, LLDB_INVALID_LINE_NUMBER, 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 CMICmdCmdExecStep::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.