[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

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 ↗

(On Diff #150652)

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 ↗

(On Diff #150652)

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

[polyakov.alex]

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

524 ↗

(On Diff #150652)

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.

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
+# Check that exec-step can step-in in a selected thread.
+# CHECK: ^running
+# CHECK: *stopped,reason="end-stepping-range"

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().GetThreadByID(nThreadId);
+    if (sbThread.IsValid())
+      sbThread.StepInto(nullptr, LLDB_INVALID_LINE_NUMBER, error);
+    else {
+      SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_THREAD_INVALID),
+                                     m_cmdData.strMiCmd.c_str(),
+                                     m_constStrArgThread.c_str()));
+      return MIstatus::failure;
+    }
+  } else rSessionInfo.GetProcess().GetSelectedThread().StepInto(
+             nullptr, LLDB_INVALID_LINE_NUMBER, error);
+
+  if (error.Success())
     return MIstatus::success;
+
+  SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_FNFAILED),
+                                 m_cmdData.strMiCmd.c_str(),
+                                 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.