[LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.

[polyakov.alex]

Summary

Now it's possible to set breakpoints before selecting a target, they will be set to the dummy target and then copied to an each added one.

[polyakov.alex]

tools/lldb-mi/MICmdCmdBreak.cpp

166 ↗

(On Diff #145721)

I want to discuss is it a good way to use 'pending' breakpoint's here?

[clayborg]

Add tests and this will be good to go.

tools/lldb-mi/MICmdCmdBreak.cpp

166 ↗

(On Diff #145721)

This looks fine.

[polyakov.alex]

Added testcase and updated revision.

[labath]

Out of curiosity, are there any plans to improve the lldb-mi test reliability situation? As it stands now, most of the lldb-mi tests are disabled one way or another due to them being flaky.

[aprantl]

In D46588#1092884, @labath wrote:

Out of curiosity, are there any plans to improve the lldb-mi test reliability situation? As it stands now, most of the lldb-mi tests are disabled one way or another due to them being flaky.

Thanks for bringing that up. I just looked at a few lldb-mi testcases and they all seem to follow a pattern of self.runCmd() followed by self.expect(). That in itself doesn't look like a particularly bad design to me since it synchronizes commands and expected output tightly. Do we know why the tests are flakey? Do they get out of sync, or is there something else?

[labath]

In D46588#1093054, @aprantl wrote:

In D46588#1092884, @labath wrote:

Out of curiosity, are there any plans to improve the lldb-mi test reliability situation? As it stands now, most of the lldb-mi tests are disabled one way or another due to them being flaky.

Thanks for bringing that up. I just looked at a few lldb-mi testcases and they all seem to follow a pattern of self.runCmd() followed by self.expect(). That in itself doesn't look like a particularly bad design to me since it synchronizes commands and expected output tightly. Do we know why the tests are flakey? Do they get out of sync, or is there something else?

Short answer: I don't know. Noone was motived enough to get to the bottom of that.

I tried looking at it once or twice, but I was put off by:

• when the test fails you get a dump of the pexpect state, which is hard to comprehend
• I tried enabling some logging but the log was full of useless messages because lldb-mi has a thread which does a 1 millisecond polls for events.

I think it would help if instead of pexpect, we used some driver which is aware of the basics of lldb-mi protocol. This would make logging the execution of a test easier and it would solve the XFAIL issue -- right now, if lldb-mi does not produce expected output, pexpect will just hang until it times out, hoping that the right output might come eventually. And it would also make it possible to run the tests on windows. The 1ms wait also sounds like something that should be fixed regardless of whether it is causing any flakyness or not.

[aprantl]

Okay, that's no good. What would you think about writing tests using lit & FileCheck? The lldb-mi tests should not be in the debuginfo category anyway, since they only test the alternative driver.

$ cat lldb-mi-demo.input
-file-exec-and-symbols a.out
-break-insert -f main
...

$ cat lldb-mi-demo.test
RUN: %cc test.c -g
RUN: %lldb-mi <lldb-mi-demo.input | FileCheck %s
CHECK: \^done
CHECK: \^done,bkpt={number=\"1\"")
...

[labath]

My main question would be whether it is possible to specify all the lldb-mi commands up-front, which is kinda required for this approach. I was against this for the lldb-server tests, because there we often deal with values which are not sufficiently deterministic (for example, because they represent addresses in the inferior address space), so we needed the know the output of one command before we could specify the next one. I felt that trying to capture this statically would end up with a reinvention of a domain-specific scripting language.

*However*, the situation may be better in lldb-mi, as here we are sitting on top of layers which will enable us to reason about things symbolically. In a random sample of lldb-mi tests I did not find any inter-command dependencies, *except* the "-file-exec-and-symbols", where we pass the full path of the built executable. This can probably be worked around somehow.

[aprantl]

Can you give an example of what you mean by "specifying the lldb-mi commands up-front"? it's not immediately obvious to me how that would look like.

[labath]

In D46588#1093267, @aprantl wrote:

Can you give an example of what you mean by "specifying the lldb-mi commands up-front"? it's not immediately obvious to me how that would look like.

In your example lldb-mi-demo.input is static text, right? No command depends on the result of any previous commands. That's what I mean by "up-front".

The opposite of that would be if you used the output of one command to construct the next one. Something like:

result = execute("first command")
value = extract_something_interesting(result)
result2 = execute(create_second_command(value))

My point is that lit+FileCheck is good if all your tests look like your example, but it's not if you need to do something like the above. I haven't looked at lldb-mi in much detail, so I can't which of these two options is correct, although it seems that at least the majority of the tests could be written that way.

[aprantl]

Got it. We certainly do have dependent checks in our current tests:

# Test that -data-info-line works for address
self.runCmd("-data-info-line *%#x" % addr)
self.expect(
    "\^done,start=\"0x0*%x\",end=\"0x[0-9a-f]+\",file=\".+?main.cpp\",line=\"%d\"" %
    (addr, line))

# Test that -data-info-line works for file:line
self.runCmd("-data-info-line main.cpp:%d" % line)
self.expect(
    "\^done,start=\"0x0*%x\",end=\"0x[0-9a-f]+\",file=\".+?main.cpp\",line=\"%d\"" %
    (addr, line))

[aprantl]

Actually, the example I posted was not a good one. I'm fairly certain that that was de facto static information or otherwise not particularly important information.

[polyakov.alex]

So, what do you think about the patch? There is no any decision about lldb-mi tests and I do not know what to do with this.

[aprantl]

The code looks good — I think it would be worthwhile to try and convert this test to a LIT test. You can try to model it after lit/Expr/TestCallUserDefinedFunction.test. You will also need to modify lit/lit.cfg to define a %lldb-mi variable, but that should be straightforward. Let me know if that works. If we run into too many problems we can stick to the expect-style tests for now, but I'd like to do this experiment at the very beginning so all the subsequent patches can benefit from it.

packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py

157 ↗

(On Diff #145909)

since this comes form the dummy target, I suppose this doesn't matter?

[polyakov.alex]

Moved tests to lit.

[polyakov.alex]

Added comments describing which lldb-mi command's output is currently parsing.

[aprantl]

Thanks for converting the test!

lit/Breakpoint/break-insert.test

2 ↗

(On Diff #146489)

Please add a comment like:
# Test that a breakpoint can be inserted before creating a target.

5 ↗

(On Diff #146489)

Very nice!

What does the actual output format of lldb-mi look like?
If every reply is on a single line, we could make this test even stricter by using CHECK-NEXT instead of CHECK to make sure that no other output appears between two matches. Alternatively you could also insert a CHECK-NOT: ^done line before every command to make the test even stricter.

[polyakov.alex]

Added more comments about the test.

[aprantl]

Thanks, LGTM.

[polyakov.alex]

lit/Breakpoint/break-insert.test

5 ↗

(On Diff #146489)

Unfortunately, lldb-mi output may look like:

(gdb)
-exec-run
^running

thread-group-started,id="i1",pid="4632"

thread-created,id="1",group-id="i1"

thread-selected,id="1"

(gdb)

library-loaded,id="/lib/x86_64-linux-gnu/ld-2.23.so",target-name="/lib/x86_64-linux-gnu/ld-2.23.so",host-name="/lib/x86_64-linux-gnu/ld-2.23.so",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/ld-2.23.so",loaded_addr="-",size="0"

(gdb)

library-loaded,id="[vdso]",target-name="[vdso]",host-name="[vdso]",symbols-loaded="1",symbols-path="",loaded_addr="0x00007ffff7ffa000",size="0"

breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000041eed0",func="??",file="??",fullname="??/??",line="0",pending=["main"],times="0",original-location="main"}

(gdb)
(gdb)
(gdb)

library-loaded,id="/bin/bash",target-name="/bin/bash",host-name="/bin/bash",symbols-loaded="0",loaded_addr="-",size="0"

(gdb)
*running,thread-id="all"
(gdb)
(gdb)

library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"

(gdb)

library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"

(gdb)

library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"

(gdb)

library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"

library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"

library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"

(gdb)
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={level="0",addr="0x000000000041eed0",func="main",args=[],file="??",fullname="??",line="-1"},thread-id="1",stopped-threads="all"
(gdb)

So, I think, it's not a best idea to write 'CHECK-NOT' for each line.

[polyakov.alex]

lit/Breakpoint/break-insert.test

5 ↗

(On Diff #146489)

Ah, my fault, there will not be such amount of lines if we use test's binary.

[labath]

lit/Breakpoint/Inputs/break-insert.input

1–3 ↗

(On Diff #146693)

Based on my experiments, lldb-mi seems to ignore lines starting with #. If that's true then we could put the contents of this file directly in the test, which will help readability as there is only one file to open to understand what a test does.

lit/Breakpoint/break-insert.test

1 ↗

(On Diff #146693)

I think the lldb-mi tests should be kept separate. Could we move this test into a different directory? Maybe lit/tools/lldb-mi ?

[polyakov.alex]

Created separate directory for an lldb-mi tests.

[polyakov.alex]

Also, I combined FileCheck commands and lldb-mi input in a single file.

[labath]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

I'm not familiar with this directive. Are you sure that this actually does anything?

[polyakov.alex]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

I tried to use only CHECK directive, but got errors like pattern not found, so I decided that it may be caused due to CHECK search features, for example, as I know, it finds pattern from the start of the file. If we want to check lldb-mi output, we should follow a specific order.

In our case, we should find "^done" string directly after -break-insert command's output.

[labath]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

I think you got the FileCheck operation wrong.
a CHECK should always start matching from the previous match. The reason that this is passing for you now is that CHECK-AFTER is a non-existing directive and FileCheck ignores it (try replacing it with a bogus string and see if it still passes). if CHECK is not working for you here then you probably have the pattern wrong.

[polyakov.alex]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

You are right about the CHECK-AFTER. I wrote the test using only check and found an issue:
after executing of -exec-run command, we expect to see output like:
*stopped,reason="breakpoint-hit"
but, I think, that FileCheck check file for the pattern, while a -exec-run hasn't finished yet. It means that there will not be expected output and we'll get the error: no such pattern.

Is there a mechanism to add some delay to FileCheck to wait until -exec-run finished?

[labath]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

There shouldn't be a need for anything like that. FileCheck will wait until EOF before doing anything. In fact, if you try running the test script manually (lldb-mi <break-insert.test), you will see that lldb-mi in fact does *not* print out the *stopped line. What I expect is happening here is that lldb-mi reaches EOF, and then decides there is nothing left for it to do and exits (a somewhat reasonable assumption given that it's expecting to be talking via an interactive link).

This sounds like a fairly fundamental problem with the lit+FileCheck testing strategy. If we're not able to come up with a command to make lldb-mi wait until the target stops (maybe there is one already? I know very little about lldb-mi), we may have to revisit the whole testing strategy...

[aprantl]

If we're not able to come up with a command to make lldb-mi wait until the target stops (maybe there is one already? I know very little about lldb-mi), we may have to revisit the whole testing strategy...

If one doesn't exist then I think it would be reasonable to invent one. Handling an additional command that is used in testing only should not break any existing clients.

[clayborg]

In D46588#1099536, @aprantl wrote:

If we're not able to come up with a command to make lldb-mi wait until the target stops (maybe there is one already? I know very little about lldb-mi), we may have to revisit the whole testing strategy...

If one doesn't exist then I think it would be reasonable to invent one. Handling an additional command that is used in testing only should not break any existing clients.

I am not sure I like the direction of "lets test lldb-mi in a way that doesn't behave like a real debug session" by making new testing stuff so we can text scrape.

[aprantl]

In D46588#1099566, @clayborg wrote:

In D46588#1099536, @aprantl wrote:

If we're not able to come up with a command to make lldb-mi wait until the target stops (maybe there is one already? I know very little about lldb-mi), we may have to revisit the whole testing strategy...

If one doesn't exist then I think it would be reasonable to invent one. Handling an additional command that is used in testing only should not break any existing clients.

I am not sure I like the direction of "lets test lldb-mi in a way that doesn't behave like a real debug session" by making new testing stuff so we can text scrape.

There are two levels that we want to test lldb-mi on:

1. Test on the functionality / specification level: We want to make sure that a specific lldb-mi command triggers a specific action. To test this as reliably as possible getting rid of any temporal uncertainty is a good thing. This is what I'm concerned with here. The idea is to serialize an entire debug session with fixed inputs into one output file, which can then be checked via FileCheck. You are right that this is "scraping text", but I do think that that is the appropriate abstraction level for testing a textual protocol. The main befit of doing it this way is that there is no pexpect and timeouts involved (other than the per-test timeout imposed by the lit driver). This way we get a very reliable testsuite that is not affected by how much load the machine running it is under.

2. Test end-to-end that lldb-mi integrates well with an interactive client. I don't actually have any good ideas for how to do this beyond the existing pexpect-based method, which, as we found out, has some reliability issues. But perhaps reducing the number of interactive tests in favor of mostly specification (1) tests will reduce the chances of a random timeout occurring to a manageable number.

[jingham]

For #2 we should be able to come up with something much more trivial than pexpect (and thereby more reliable) because we really aren't expecting patterns. We are always sending a complete string reading a complete string back, then checking the string against some pattern. It should be pretty straightforward to write a lit tool that does that. IIUC that's what Davide did for the swift REPL tests in the github repo.

[polyakov.alex]

lit/tools/lldb-mi/breakpoint/break-insert.test

10 ↗

(On Diff #146777)

I tried to run script manually (lldb-mi < break-insert.test) and got output which isn't useful for testing -break-insert command since there isn't any information about hitting breakpoint.

~/workspace/gsoc/build/bin/lldb-mi < ../break-insert.test 
(gdb)
^done
(gdb)
^done
(gdb)
^done
(gdb)
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0xffffffffffffffff",func="??",file="??",fullname="??/??",line="0",pending=["breakpoint"],times="0",original-location="breakpoint"}
(gdb)
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0xffffffffffffffff",func="??",file="??",fullname="??/??",line="0",pending=["breakpoint"],times="0",original-location="breakpoint"}
(gdb)
^done
(gdb)
^done
(gdb)
=library-loaded,id="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",target-name="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",host-name="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",symbols-loaded="0",loaded_addr="-",size="0"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00000000004004c6",func="breakpoint",file="break-insert.c",fullname="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/break-insert.c",line="2",pending=["breakpoint"],times="0",original-location="breakpoint"}
(gdb)
^done
(gdb)
^done
(gdb)
^running
=thread-group-started,id="i1",pid="13757"
(gdb)
=thread-created,id="1",group-id="i1"
=thread-selected,id="1"
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/ld-2.23.so",target-name="/lib/x86_64-linux-gnu/ld-2.23.so",host-name="/lib/x86_64-linux-gnu/ld-2.23.so",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/ld-2.23.so",loaded_addr="-",size="0"
(gdb)
=library-loaded,id="[vdso]",target-name="[vdso]",host-name="[vdso]",symbols-loaded="1",symbols-path="",loaded_addr="0x00007ffff7ffa000",size="0"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00000000004004c6",func="breakpoint",file="break-insert.c",fullname="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/break-insert.c",line="2",pending=["breakpoint"],times="0",original-location="breakpoint"}
(gdb)
(gdb)
=library-loaded,id="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",target-name="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",host-name="/home/alexander/workspace/gsoc/llvm/tools/lldb/lit/tools/lldb-mi/breakpoint/inputs/a.out",symbols-loaded="0",loaded_addr="-",size="0"
(gdb)
*running,thread-id="all"
(gdb)
^done
(gdb)
^error,message="error: Process is running.  Use 'process interrupt' to pause execution."
(gdb)
^done
(gdb)

[aprantl]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

CHECK-AFTER is not recognized by FileCheck:

https://www.llvm.org/docs/CommandGuide/FileCheck.html

You probably saw this in a testcase that ran FileCheck twice, one time with the CHECK prefix and once with a custom --check-prefix=CHECK-AFTER which is a common trick to have more than one set of FileCheck directives in a single file.

[polyakov.alex]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

Yes. There is no problem to write test using only CHECK and CHECK-NOT, but as I said, in lldb-mi's output we can't find any info about hitting breakpoint, so the question is: is it enough to check that breakpoint was set to a selected target?

[aprantl]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

in lldb-mi's output we can't find any info about hitting breakpoint,

Is that how the gdb/mi protocol is supposed to work or is that a bug or missing feature in lldb-mi?

so the question is: is it enough to check that breakpoint was set to a selected target?

If that's just how the protocol works then we'll have to make do with what we got.

[labath]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

That's not "how the protocol works" in general. It's how lldb-mi behaves when it's control connection is closed. If you pipe its input from a file, lldb-mi will get an EOF as soon as it processes the last command, interpret that as the IDE closing the connection and exit (a perfectly reasonable behavior for its intended use case). So it will never get around to printing the breakpoint-hit message, because it will not wait long enough for that to happen. If you make sure the stdin does not get an EOF then (either by typing the same commands interactively, or by doing something like cat break_insert.test - | lldb-mi, you will see the "hit" message does get displayed (however, then the lldb-mi process will hang because it will expect more commands).

[aprantl]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

To make sure I understand your point: Does lldb-mi consumes input asynchronously from its command handler, so for example, when sending the mi equivalent of b myFunc, r, p foo — will lldb-mi wait until the breakpoint is hit or the target is terminated before consuming the next command after r or will it consume and attempt to execute p foo as soon as possible and thus potentially before the breakpoint is hit?

If this is the case, we could introduce a "synchronized" mode for testing lldb-mi that behaves more like the SBAPI. Or we could pick up the idea you mentioned a while ago and produce a separate lldb-mi test driver that can send and get a reply for exactly one action at a time, similar to how the SBAPI behaves.

[aprantl]

Alexander, I don't want this discussion to block you progress too much. It's important that we sort out the best way to test lldb-mi early on (and I hope this doesn't take too long), but while we are debating this you could also land the patch with the classic expect-based testcase that you had and go back and convert it later on. It's a bit more work, but converting the testcases shouldn't be too difficult.

[labath]

lit/tools/lldb-mi/breakpoint/break-insert.test

14 ↗

(On Diff #146777)

To make sure I understand your point: Does lldb-mi consumes input asynchronously from its command handler, so for example, when sending the mi equivalent of b myFunc, r, p foo — will lldb-mi wait until the breakpoint is hit or the target is terminated before consuming the next command after r or will it consume and attempt to execute p foo as soon as possible and thus potentially before the breakpoint is hit?

I know very little about lldb-mi implementation or the gdb-mi protocol, but yes, that is what I think is happening. In general, you need the ability to issue asynchronous commands, if for nothing else, then to implement the equivalent of ^C. gdb-remote protocol also multiplexes reading from the socket and waiting on inferior state change for this reason (and would respond the same way if you tried to feed it input from a file).

BTW: SB API also has an asynchronous mode, and I think it is even the default. It's just that we switch it to synchronous mode for testing, as one does not care about the ability to interrupt the process in tests (most of the time).

If this is the case, we could introduce a "synchronized" mode for testing lldb-mi that behaves more like the SBAPI. Or we could pick up the idea you mentioned a while ago and produce a separate lldb-mi test driver that can send and get a reply for exactly one action at a time, similar to how the SBAPI behaves.

I think both of these are valid options. Probably the simplest way to implement the first one would be to introduce a -wait-for-target-to-stop command (maybe there is one already ??). The advantage of the second one is that we will have the ability to inject commands which depend on the results of previous commands (something that I think we will need, sooner or later).

[polyakov.alex]

I have all the patches I was sending. I will follow this topic until you decide what to do with lldb-mi testing.

[aprantl]

The advantage of the second one is that we will have the ability to inject commands which depend on the results of previous commands (something that I think we will need, sooner or later).

That is worth considering. To write good tests that depend on previous results, we probably want to have SBAPI-like Python scripting for lldb-mi available. To make that work we would need to introduce an lldbmi.HandleCommand("-mycmd...") interface that runs one command, blocks on it, and returns the output in a string. And then we would need to export that to Python. Since the interface is string-in-string-out, we could probably do it via C without even involving SWIG. As an escape hatch to access features outside of the mi command set, we could implement the -interpreter-exec mi command that passes its input to SBAPI's HandleCommand() if need be.
Alternatively we could use gtest to write unittest-style lldb-mi tests directly in C++, if going via Python is too messy. Again, we would do that in a blocking or synchronized I/O mode.

[labath]

In D46588#1102363, @aprantl wrote:

The advantage of the second one is that we will have the ability to inject commands which depend on the results of previous commands (something that I think we will need, sooner or later).

That is worth considering. To write good tests that depend on previous results, we probably want to have SBAPI-like Python scripting for lldb-mi available. To make that work we would need to introduce an lldbmi.HandleCommand("-mycmd...") interface that runs one command, blocks on it, and returns the output in a string. And then we would need to export that to Python. Since the interface is string-in-string-out, we could probably do it via C without even involving SWIG. As an escape hatch to access features outside of the mi command set, we could implement the -interpreter-exec mi command that passes its input to SBAPI's HandleCommand() if need be.
Alternatively we could use gtest to write unittest-style lldb-mi tests directly in C++, if going via Python is too messy. Again, we would do that in a blocking or synchronized I/O mode.

I agree with your analysis. I don't want to give an opinion on the direction, because:
a) it's not clear to me which way to go, each possibility has different tradeoffs involved
b) I don't see myself doing anything related to lldb-mi any time soon

What I can offer as an additional data point is the lldb-server test suite. lldb-server very similar to lldb-mi in a way, as it has an almost-text-based protocol as its main form of interaction with the world. We also have two ways of testing it: a python one (test/testcases/tools/lldb-server and a gtest one (unittests/tools/lldb-server). the python one is older, but a bit messy (IMO). The gtest one was started because we had a person interested in improving lldb-server testing situation. I supported starting a fresh sub-test suite, because I saw it as a way to solve a couple some fundamental issues in the old one:

• code duplication: the python test suite needs a complete reimplementation of the gdb-remote protocol. the c++ test can just hook it to the existing functionality at an appropriate layer.
• remote testing mess: a very convoluted setup is required to run tests on a remote (android) device. It's weird that you have to build a host lldb and run the tests from there, even though the lldb-server and the executables it debugs run on a different host/arch. The python part of the test needs to run on the host (there's no easily available python for android), so there's a lot of flakyness and complexity coming from the port forwarding/network communication).

The c++ based approach could solve these, as the gtest executable could run on the same device as the lldb-server under test, and it would be possible to run the tests via check-lldb(-server?) from the cross-build tree instead of copying things around between two build trees.

Sadly, the person who was supposed to do this left the team. I have tried to carry this idea on slowly, but haven't yet gotten around to implementing the most important part (running tests remotely), so I couldn't start migrating the tests. In retrospect, I am less excited about this idea than I was a year ago, though I still think it would offer a better testing story for lldb-server.

Overall, I am not sure how much of this relates to lldb-mi (for example, you probably aren't interested in running it in such a constrained environment), but I thought it might be interesting for you to look at these before deciding on a direction. I also don't want to hold up Alexander's internship over this, as I'm not sure this is what he signed up for. I wanted to bring up the testing story, because I think there are some issues there, but I'll leave it up to you (for some definition of "you") to decide on what to do about that.

[aprantl]

My overall mental image of lldb-mi is very incomplete, but I'm imagining lldb-mi as having one thread that wakes up every n milliseconds checks for command input and then calls into the SBAPI to handle the commands. If that is how it works, then one very simple thing we can try is to call SBDebugger::SetAsync(false), thus forcing all the SBAPI calls to block until completion.

Alexander, can you see what happens to your lit test when you enable synchronous mode in the SBAPI? Does it block now as expected, or does it still finish early? If that experiment works, we could add a --async command line option to lldb-mi to enable this behavior for running the test suite.

[polyakov.alex]

In D46588#1103180, @aprantl wrote:

My overall mental image of lldb-mi is very incomplete, but I'm imagining lldb-mi as having one thread that wakes up every n milliseconds checks for command input and then calls into the SBAPI to handle the commands. If that is how it works, then one very simple thing we can try is to call SBDebugger::SetAsync(false), thus forcing all the SBAPI calls to block until completion.

Alexander, can you see what happens to your lit test when you enable synchronous mode in the SBAPI? Does it block now as expected, or does it still finish early? If that experiment works, we could add a --async command line option to lldb-mi to enable this behavior for running the test suite.

Where is the right place to call SBDebugger::SetAsync(false) to enable synchronous mode?

[aprantl]

For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().

[clayborg]

In D46588#1103237, @aprantl wrote:

For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().

But don't do it here permanently...

[polyakov.alex]

In D46588#1103237, @aprantl wrote:

For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().

Yep, after that, test is passing with only CHECK directives.

[aprantl]

Okay, that sounds promising! Then let's proceed this way:

• Add a new command line option to lldb-mi that is called --synchronous with a help text "Block until each command has finished executing. Used for testing only." and use it in this test
• continue writing as many lldb-mi tests as possible (everything where the commands don't depend on replies) using the lit/FileCheck approach
• gradually convert existing pexpect tests over to lit/FileCheck
• convert the the remaining tests to use synchronous mode, which might improve the reliability a tiny bit. We will still run into situations where on very loaded systems the pexpect timeout is triggered before a reply comes back from lldb-mi, but it will be less likely.

[polyakov.alex]

This version of commit uses the new lldb-mi option that I added to review https://reviews.llvm.org/D47110.

[aprantl]

LGTM!

[aprantl]

lit/lit.cfg

61 ↗

(On Diff #147681)

It looks like "lldb-mi" may not be a valid substitution. On Darwin the command

# RUN: %lldb_mi

is expanded to

bin/lldb -S .../llvm/tools/lldb/lit/lit-lldb-init_mi < ...

So it behaves like %lldb+_mi.

[aprantl]

lit/lit.cfg

61 ↗

(On Diff #147681)

Can you see if you can figure out what's going on? Perhaps we need to rename it to lldbmi without the underscore?

[polyakov.alex]

lit/lit.cfg

61 ↗

(On Diff #147681)

On linux it's ok. I'll remove underscore and be appreciated if you check it on Darwin.

[polyakov.alex]

Removed underscore in the lldb-mi variable from lit.cfg.

lldb/trunk/lit/lit.cfg

 
 # Register substitutions
 config.substitutions.append(('%python', config.python_executable))
 
 debugserver = lit.util.which('debugserver', lldb_tools_dir)
 lldb = "%s -S %s/lit-lldb-init" % (lit.util.which('lldb', lldb_tools_dir),
                                config.test_source_root)
 
+lldbmi = lit.util.which('lldb-mi', lldb_tools_dir)
+
 if not os.path.exists(config.cc):
     config.cc = lit.util.which(config.cc, config.environment['PATH'])
 
 if not os.path.exists(config.cxx):
     config.cxx = lit.util.which(config.cxx, config.environment['PATH'])
 
 if platform.system() in ['Darwin']:
     try:
         out = subprocess.check_output(['xcrun', '--show-sdk-path']).strip()
         res = 0
     except OSError:
         res = -1
     if res == 0 and out:
         sdk_path = out
         lit_config.note('using SDKROOT: %r' % sdk_path)
         config.cc += " -isysroot %s" % sdk_path
         config.cxx += " -isysroot %s" % sdk_path
 
 config.substitutions.append(('%cc', config.cc))
 config.substitutions.append(('%cxx', config.cxx))
 
+config.substitutions.append(('%lldbmi', lldbmi + " --synchronous"))
 config.substitutions.append(('%lldb', lldb))
 
 if debugserver is not None:
     config.substitutions.append(('%debugserver', debugserver))
 
 for pattern in [r"\bFileCheck\b",
                 r"\blldb-test\b",
                 r"\byaml2obj\b",

lldb/trunk/lit/tools/lldb-mi/breakpoint/break-insert.test

+# RUN: %cc %p/inputs/break-insert.c -g
+# RUN: %lldbmi < %s | FileCheck %s
+
+# Test that a breakpoint can be inserted before creating a target.
+
+-break-insert breakpoint
+# CHECK: ^done,bkpt={number="1"
+
+-file-exec-and-symbols a.out
+# CHECK: ^done
+
+-exec-run
+# CHECK: ^running
+# CHECK: *stopped,reason="breakpoint-hit"
+

lldb/trunk/lit/tools/lldb-mi/breakpoint/inputs/break-insert.c

+int breakpoint() { // Breakpoint will be set here.
+  return 0;
+}
+
+int main() {
+  return breakpoint();
+}

lldb/trunk/lit/tools/lldb-mi/breakpoint/lit.local.cfg

+config.suffixes = ['.test']

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

CMICMDBASE_GETOPTION(pArgPendingBrkPt, OptionShort,
                        m_constStrArgNamedPendinfBrkPt);
   CMICMDBASE_GETOPTION(pArgDisableBrkPt, OptionShort,
                        m_constStrArgNamedDisableBrkPt);
   CMICMDBASE_GETOPTION(pArgConditionalBrkPt, OptionShort,
                        m_constStrArgNamedConditionalBrkPt);
   CMICMDBASE_GETOPTION(pArgRestrictBrkPtToThreadId, OptionShort,
                        m_constStrArgNamedRestrictBrkPtToThreadId);
 
+  // Ask LLDB for the target to check if we have valid or dummy one.
+  CMICmnLLDBDebugSessionInfo &rSessionInfo(
+      CMICmnLLDBDebugSessionInfo::Instance());
+  lldb::SBTarget sbTarget = rSessionInfo.GetTarget();
+
   m_bBrkPtEnabled = !pArgDisableBrkPt->GetFound();
   m_bBrkPtIsTemp = pArgTempBrkPt->GetFound();
   m_bHaveArgOptionThreadGrp = pArgThreadGroup->GetFound();
   if (m_bHaveArgOptionThreadGrp) {
     MIuint nThreadGrp = 0;
     pArgThreadGroup->GetExpectedOption<CMICmdArgValThreadGrp, MIuint>(
         nThreadGrp);
     m_strArgOptionThreadGrp = CMIUtilString::Format("i%d", nThreadGrp);
   }
+
+  if (sbTarget == rSessionInfo.GetDebugger().GetDummyTarget())
+    m_bBrkPtIsPending = true;
+  else
     m_bBrkPtIsPending = pArgPendingBrkPt->GetFound();
+
   if (pArgLocation->GetFound())
     m_brkName = pArgLocation->GetValue();
   else if (m_bBrkPtIsPending) {
     pArgPendingBrkPt->GetExpectedOption<CMICmdArgValString, CMIUtilString>(
         m_brkName);
   }
   if (pArgIgnoreCnt->GetFound()) {
     pArgIgnoreCnt->GetExpectedOption<CMICmdArgValNumber, MIuint>(
bool CMICmdCmdBreakInsert::Execute() {
 
   // Break defined as an function
   if (eBrkPtType == eBreakPoint_NotDefineYet) {
     eBrkPtType = eBreakPoint_ByName;
   }
 
   // Ask LLDB to create a breakpoint
   bool bOk = MIstatus::success;
-  CMICmnLLDBDebugSessionInfo &rSessionInfo(
-      CMICmnLLDBDebugSessionInfo::Instance());
-  lldb::SBTarget sbTarget = rSessionInfo.GetTarget();
   switch (eBrkPtType) {
   case eBreakPoint_ByAddress:
     m_brkPt = sbTarget.BreakpointCreateByAddress(nAddress);
     break;
   case eBreakPoint_ByFileFn: {
     lldb::SBFileSpecList module; // search in all modules
     lldb::SBFileSpecList compUnit;
     compUnit.Append(lldb::SBFileSpec(fileName.c_str()));

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

 //------------------------------------------------------------------------------------
 // Details: Get current target.
 // Type:    Method.
 // Args:    None.
 // Return:  lldb::SBTarget   - current target.
 // Throws:  None.
 //--
 lldb::SBTarget CMICmnLLDBDebugSessionInfo::GetTarget() const {
-  return GetDebugger().GetSelectedTarget();
+  auto target = GetDebugger().GetSelectedTarget();
+  if (target.IsValid())
+    return target;
+  return GetDebugger().GetDummyTarget();
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: Get current process.
 // Type:    Method.
 // Args:    None.
 // Return:  lldb::SBProcess   - current process.
 // Throws:  None.
 //--
 lldb::SBProcess CMICmnLLDBDebugSessionInfo::GetProcess() const {
   return GetTarget().GetProcess();
 }