Page MenuHomePhabricator

lldb-server tests: Add support for testing debugserver
ClosedPublic

Authored by labath on Jul 12 2017, 9:46 AM.

Details

Summary

This adds support for running the lldb-server test suite (currently consisting
of only one test) against the debugserver. Currently, the choice which binary
to test is based on the host system. This will need to be replaced by something
more elaborate if/when lldb-server starts supporting debugging on darwin.

I need to make a couple of tweaks to the test client to work with debugserver:

  • debugserver has different command-line arguments - launching code adjusted to handle that
  • debugserver sends duplicate "medata" fields in the stop reply packet - adjusted stop-reply parsing code to handle that
  • debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jul 12 2017, 9:46 AM

@beanz: It looks like you're doing some debugserver work. :) Any thoughts on this?

This isn't about this patch, but replying to:

debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Especially when dealing with flakey connections, knowing that you are done with a target was really handy. People what to hit "rerun" in some UI and have the rerun happen instantly, but if you have no way of knowing when the former process actually died, you have to introduce delays, which slows this down. This is particularly true when talking to an OS that really wants only one copy of an "app" running at a time (e.g. iOS) but is more generally useful for responsiveness. And if something goes wrong with killing the target, we could even use the new error reply strings to give more info about this.

This isn't about this patch, but replying to:

debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Especially when dealing with flakey connections, knowing that you are done with a target was really handy. People what to hit "rerun" in some UI and have the rerun happen instantly, but if you have no way of knowing when the former process actually died, you have to introduce delays, which slows this down. This is particularly true when talking to an OS that really wants only one copy of an "app" running at a time (e.g. iOS) but is more generally useful for responsiveness. And if something goes wrong with killing the target, we could even use the new error reply strings to give more info about this.

Yes, I can totally believe that the response to that packet can be useful. I'm not actually sure that changing the debugserver behavior is the best solution here, I just think that we should pick one behavior and have both of the stubs act the same way. It's a bit unfortunate that this is in violation of the gdb-remote protocol, but I guess if a client is expecting the connection to be dropped, it is unlikely it will get confused by an OK response (OTOH, maybe we could use the fact that the connection was dropped as a signal that the kill has completed).

beanz accepted this revision.Aug 3 2017, 10:57 AM

This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.

I have one specific comment below that is along those lines. Otherwise, this LGTM.

unittests/tools/lldb-server/tests/TestClient.cpp
32 ↗(On Diff #106247)

Instead of making this ifdef'd can you just check to see if LLDB_SERVER is debugserver or lldb-server?

One of the items on my todo list is to get rid of all the places where #ifdef __APPLE__ equates to using debugserver so that we can start evaluating making lldb-server work on Darwin.

This revision is now accepted and ready to land.Aug 3 2017, 10:57 AM

This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.

I have one specific comment below that is along those lines. Otherwise, this LGTM.

I did consider this to some depth while making this change. I think that the easiest way to make that happen would be to create two cmake targets (add_lldb_unittest(LLDBServerTests..., add_lldb_unittest(DebugServerTests...). The main difference between these would be the value of the LLDB_SERVER define, but other differences are possible as well (for example, if only one of the servers implements some feature). It means we will compile some things twice, but in return it will integrate nicely with the existing llvm unit testing infrastructure.

It may be possible to use the googletest's parameterized test mechanism for this, but I don't know that well enough to tell. In any case, this shouldn't interfere with this patch -- the only thing that will change is that the return value of isdebugserver() will be determined through some runtime mechanism.

unittests/tools/lldb-server/tests/TestClient.cpp
32 ↗(On Diff #106247)

I'll change this to detect the server type based on the file name.

This revision was automatically updated to reflect the committed changes.