This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by polyakov.alex on Jun 5 2018, 1:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

polyakov.alex created this revision.Jun 5 2018, 1:50 PM
aprantl accepted this revision.Jun 5 2018, 3:58 PM
aprantl added a reviewer: stella.stamenova.
aprantl added inline comments.
lit/tools/lldb-mi/exec/exec-next.test
2 ↗(On Diff #150035)

@stella.stemanova: Would be interesting to understand why (/if ?) this doesn't work on windows

This revision is now accepted and ready to land.Jun 5 2018, 4:00 PM
clayborg accepted this revision.Jun 5 2018, 4:38 PM
lit/tools/lldb-mi/exec/exec-next.test
2 ↗(On Diff #150035)

I can test it out after it's committed.

labath added a subscriber: labath.Jun 6 2018, 12:45 AM
labath added inline comments.
tools/lldb-mi/MICmdCmdExec.cpp
384 ↗(On Diff #150035)

It looks like this branch is not covered by the test. Also, do we care about what happens when the client specifies an invalid thread id?
(I'm mainly pointing these out because these seem like they would be hard to test in the current framework, the thread ids being unpredictable and all.)

polyakov.alex added a reviewer: labath.

Added some checks for invalid thread id.

polyakov.alex added inline comments.Jun 6 2018, 8:05 AM
tools/lldb-mi/MICmdCmdExec.cpp
384 ↗(On Diff #150137)

We can't test this branch until we have a way to get thread ID and then store it in some variable.

aprantl accepted this revision.Jun 6 2018, 8:54 PM
aprantl added inline comments.
lit/tools/lldb-mi/exec/exec-next.test
19 ↗(On Diff #150137)

0 feels like it might actually be a valid thread id on some systems.. perhaps use really high number instead?

labath added inline comments.Jun 7 2018, 1:36 AM
lit/tools/lldb-mi/exec/exec-next.test
19 ↗(On Diff #150137)

I was surprised by that as well, so I tried a some experiments. I don't know how or why, but lldb-mi seems to use it's own notion of thread-ids, which are independent of os-level ids and always start with one. I guess they are just indexes into the list of threads. I don't know if that is intentional or what.

tools/lldb-mi/MICmdCmdExec.cpp
384 ↗(On Diff #150137)

Yes, that's the issue I was alluding to. I am not going to block this patch over it or anything, but I want to make sure you are aware that you're hitting the limitations of the FileCheck test approach already.

That said, if what I said above about the thread-id's always being numbered starting from one is true, then the thread ids may be predictable enough to test using -exec-next --thread 1 and avoid this problem for now.

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

labath accepted this revision.Jun 7 2018, 7:30 AM

looks good to me

This revision was automatically updated to reflect the committed changes.