[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

Fri May 12 15:52:34 PDT 2017

labath added a comment.

next batch of comments from me (I expect to have more on monday). :)

In https://reviews.llvm.org/D32930#752843, @krytarowski wrote:

> I can build locally with `make thread_inferior`, how to run it?

run the `check-lldb-unit` target.

================
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+  LLVM_BUILTIN_DEBUGTRAP;
+  delay = false;
----------------
krytarowski wrote:
> jmajors wrote:
> > krytarowski wrote:
> > > jmajors wrote:
> > > > zturner wrote:
> > > > > This will work on MSVC and presumably clang.  I'm not sure about gcc.  Is that sufficient for your needs?   Do you know if gcc has the `__builtin_debugtrap` intrinsic?
> > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an issue. If we ever change our compiler of choice, we can always change this to match.
> > > Yes, we use and support GCC with libstdc++ to build LLDB including tests. At least on NetBSD.
> > Is there a gcc equivalent, that I could wrap in some #ifdefs?
> No, there is no equivalent and I'm trying to convince that we should not try to use this `__builtin_debugtrap()` in the code. We should ask the debugger to set and handle the trap.
> 
> Otherwise we will need to handle this on per-cpu and per-os matrix. In the SPARC/NetBSD example we would need to ask the debugger to set PC after the trap manually.
The thing with the lldb-server tests is that there is no "debugger" which can set the figure out where to set the breakpoint. Lldb-server operates at a much lower level, and it knows nothing about dwarf, or even symbol tables, and I think the tests shouldn't either (mainly to keep the tests more targetted, but also because it would be quite tricky to wire that up at this level). The existing lldb-server tests don't do debug info parsing either.

BTW, this test doesn't actually need the int3 breakpoint for its work -- all we need is for the inferior to stop so that the debugger can take a look at this state. Any stopping event will do the trick, and the most "portable" would probably be dereferencing a null pointer.

However, we will get back to this soon enough, when we start talking about breakpoint-setting tests. Since the client doesn't know anything about debug info, the best way to figure out where to set a breakpoint is for the inferior to tell us. The way existing lldb-server tests do that is via printf, but that has some issues (intercepting stdio is hard or impossible on windows and stdio comes asynchronously on linux so it is hard to write race-free tests). The most reliable way I came up for that was to put that value in a register. Now this requires a bit of assembly (eg., `movq %rax, $function_I_want_to_break_in; int3` in x86 case), but that little snippet can be tucked away in a utility function (plus a similar utility function on the recieving side to read out the value) and noone has to look at it again, and the rest of the test can be architecture-independent.

The assembly will obviously be architecture-dependent, but I don't think we will really need an OS dimension. I am not sure if we currently support on os where the PC fixup would be necessary, but even if we do, the implementation of that would be quite simple.

I am open to other ideas on how to pass information between the inferior and the test though.

================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:91
+    if (!register_dict) {
+      return std::shared_ptr<JThreadsInfo>(nullptr);
+    }
----------------
return nullptr; (I.e., a shared pointer is implicitly constructible from nullptr).

================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+    llvm::support::endianness endian) {
+  auto stop_reply = std::shared_ptr<StopReply>(new StopReply());
+
----------------
zturner wrote:
> Can you use `llvm::make_shared<StopReply>()` here?  Probably doesn't matter much, but it's good practice to use `make_shared<>` wherever possible since it uses less memory.
I raise the bet to `llvm::make_unique` ;). shared pointers lead to messy ownership semantics, we should only use them when absolutely necessary, and I don't think that is the case here.

================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+    if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+      val.getAsInteger(16, stop_reply->thread);
+      key.substr(1, 2).getAsInteger(16, stop_reply->signal);
----------------
zturner wrote:
> Is error checking needed here?
More error checking definitely needed (here and everywhere else). If I break lldb-server while working on it, I'd like to see a clear error message from the test rather than an obscure crash. This should return `Expected<StopReply>` if you don't care about copying or `Expected<std::unique_ptr<StopReply>>` if you do (*), so that the test can ASSERT that the message was received and parsed correctly and print out the error otherwise.

(*) Or the out argument idea I wrote about earlier.

================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:65
+
+  sleep(5); // TODO: Sleep is bad. Can I wait for it to start?
+  SendAck(); // Send this as a handshake.
----------------
jmajors wrote:
> labath wrote:
> > Since you're using reverse-connect (which is good), you should be able to detect that the server is ready by the fact it has connected to you.
> Are you implying that that connection is a side effect of something I've called, or that there's another function to call?
> When I didn't have this sleep here, I got a very generic error on all subsequent communication.
You have a separate thread which accepts the connection (see call to StartListenThread), which is the reason sleep helps. Our API for this is a bit clunky (IIRC you have to create the TCPSocket object yourself, the Communication class does not support it), but I'd recommend going for a single-threaded approach here:
- listen, get the port
- start lldb-server
- accept connection

This way, you can have no races and the code is clear.

================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+  Host::Kill(server_process_info.GetProcessID(), 15);
+}
----------------
jmajors wrote:
> krytarowski wrote:
> > jmajors wrote:
> > > labath wrote:
> > > > This is not portable.
> > > Is there a portable way of stopping?
> > `15` on my platform (NetBSD) is `SIGTERM`.
> > 
> > I've implemented similar feature in `NativeProcessNetBSD::Halt()`. Perhaps you can call `Halt()` as well?
> I changed it to send $k and let the lldb-server figure out platform issues.
Yes, `$k` is the first thing we should try, as that lets the server shutdown cleanly. To make the test more robust in face of broken lldb-server, we may want to add a hard kill after a timeout, but there are more important things to sort first.

https://reviews.llvm.org/D32930