This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by polyakov.alex on Jun 6 2018, 11:23 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

polyakov.alex created this revision.Jun 6 2018, 11:23 AM
polyakov.alex added a comment.EditedJun 6 2018, 11:34 AM

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.

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 added a comment.Jun 7 2018, 2:56 AM

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.

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.

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

aprantl accepted this revision.Jun 7 2018, 12:17 PM
This revision is now accepted and ready to land.Jun 7 2018, 12:17 PM

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

This revision was automatically updated to reflect the committed changes.
clayborg added inline comments.Jun 11 2018, 8:27 AM
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 added inline comments.Jun 11 2018, 9:24 AM
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.