New framework for lldb client-server communication tests.

[jmajors]

Summary

This is a new C++ test framework based on Google Test, and one working
example test.
The intention is to replace the existing tests in
packages/Python/lldbsuite/test/tools/lldb-server/ with this suite
and use this framework for all new client server tests.

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.cpp

123–124

I don't see anything in either class or their ancestors that will format the number as a two nibble hex value.

134

That container lacks a necessary constructor.

unittests/tools/lldb-server/tests/MessageObjects.h

83–88

I use one of them in the TestClient.cpp file. I could make the other local.

unittests/tools/lldb-server/tests/TestClient.cpp

66

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.

71

Is there a portable way of stopping?

[jmajors]

Made changes based on feedback.

[zturner]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

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?

unittests/tools/lldb-server/tests/MessageObjects.cpp

84–89

Hmm.. What about this?

unsigned long X;
if (!value_str.getAsInteger(X))
  return some error;
sys::swapByteOrder(X);

It shouldn't matter if you swap the bytes in the string before doing the translation, or swap the bytes in the number after doing the translation should it?

unittests/tools/lldb-server/tests/MessageObjects.h

65

Can you constructor argument does not need to be const, as StringRefs are immutable by definition.

70

Don't need explicit if there are no arguments. It's mostly useful for single argument constructors.

[jmajors]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

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.

[jmajors]

Feedback changes.

[zturner]

unittests/tools/lldb-server/tests/MessageObjects.cpp

99

Do you need to check for an error here?

106

And here

[krytarowski]

unittests/tools/lldb-server/tests/MessageObjects.cpp

201

Throws exceptions.

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.cpp

84–89

It doesn't matter when I do it. The issue with the other functions was they were converting target to host, when both were little. For string parsing, it needs target to big to string, or target to string to big.

201

I didn't change this, because I'm expecting to remove the whole function.

unittests/tools/lldb-server/tests/TestClient.cpp

77–81

I need an parameter index to pass to the command, so the classic for loop makes more sense.

[jmajors]

More feedback changes.

[krytarowski]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

2

For the clarity I would put the copyright notice in all files, including tests.

[krytarowski]

Some headers are UNIX-specific like <unistd.h>, is it possible to make these tests compatible with Windows?

[jmajors]

I cleaned up the includes.

[jmajors]

Cleaned up the includes.

[krytarowski]

unittests/tools/lldb-server/tests/TestClient.cpp

71

15 on my platform (NetBSD) is SIGTERM.

I've implemented similar feature in NativeProcessNetBSD::Halt(). Perhaps you can call Halt() as well?

[krytarowski]

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

[krytarowski]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

Yes, we use and support GCC with libstdc++ to build LLDB including tests. At least on NetBSD.

[krytarowski]

If I'm not mistaken LLVM __builtin_debugtrap is defined only for X86. It will also fail on GCC as there is no support for it.

The NetBSD buildbot in LLDB runs GCC 5.x with libstdc++.

[krytarowski]

Personally I would rely on LLDB feature to set software/hardware breakpoint in tracee, and never simulate debugtrap with some predefined value.

This will ensure portability to more CPUs and operating systems. For example Sun SPARC CPUs (at least on NetBSD) won't pass PC after software breakpoint embedded into a tracee and will loop infinitely whenever we will try to resume execution.

[zturner]

unittests/tools/lldb-server/tests/MessageObjects.cpp

62–64

As of r302875 this is no longer true.

84–89

Maybe I'm just missing something, but if you've got the string "ABCDEF" which is a little endian string, then it is supposed to represent the number 0x00EFCDAB or 15,715,755 (assuming a 4 byte number). If you parse it as big endian, which is what getAsInteger does, you're going to end up with the number 0xABCDEF00 which is 2,882,400,000, which is |AB|CD|EF|00| (on a big endian machine) or |00|EF|CD|AB| (on a little endian machine). If you then swap the bytes, you're going to end up with |00|EF|CD|AB| (on a big endian machine) or |AB|CD|EF|00| on a little endian machine. In both cases, these represent the number 15,715,755 as desired.

It's possible I'm just getting confused here, but I don't see why the above code sample doesn't work.

[krytarowski]

unittests/tools/lldb-server/tests/MessageObjects.cpp

84–89

We are also technically supposed to support PDP endian. It's part of the GDB protocol and there are scratches for it in the LLDB code. Currently we don't support these targets in LLVM, but this might change in future (GCC actively maintains these targets).

[jmajors]

More feedback changes.

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

Is there a gcc equivalent, that I could wrap in some #ifdefs?

unittests/tools/lldb-server/tests/MessageObjects.cpp

84–89

I might have been unclear.
Using swapByteOrder, when the target is little endian works. I was explaining why the read & write functions didn't work for this case.

unittests/tools/lldb-server/tests/TestClient.cpp

71

I changed it to send $k and let the lldb-server figure out platform issues.

[jmajors]

Feedback changes.

[zturner]

Getting pretty close. Sorry, still have a few more nits.

unittests/tools/lldb-server/tests/MessageObjects.cpp

26–31

Since this is in a constructor, what happens if you get a malformed response? You don't have a way to return an error here, and none of these errors are checked. Do you want to assert if this happens? Or are you ok silently dropping this kind of failure?

110

You don't need to explicitly construct a std::shared_ptr. You can just write return nullptr; There are a couple of other places above this that do the same thing.

143

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.

160–161

Is Error checking needed on these two calls?

168

Is this the same as std::string hex_id = llvm::utostr(register_id);?

190

Is error checking needed here?

unittests/tools/lldb-server/tests/TestClient.cpp

106–109

Can you use llvm formatting methods instead of std::stringstream here?

std::string message = llvm::formatv("vCont;c:{0:x-}", thread_id).str();

156–157

std::string message = llvm::formatv("qRegisterInfo{0:x-}", register_id).str();

[krytarowski]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

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.

[krytarowski]

unittests/tools/lldb-server/tests/MessageObjects.h

34

Is there an option to use gid_t?

[labath]

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

In D32930#752843, @krytarowski wrote:

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

run the check-lldb-unit target.

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

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.

unittests/tools/lldb-server/tests/MessageObjects.cpp

92

return nullptr; (I.e., a shared pointer is implicitly constructible from nullptr).

143

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.

190

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.

unittests/tools/lldb-server/tests/TestClient.cpp

66

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.

71

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.

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.cpp

26–31

I changed it to a Create with tests for success.

143

I wanted to keep my constructors protected, to force the use of Create(). make_shared doesn't work with protected constructors.

168

No. The register IDs are all two nibbles wide, so I need the setw & setfill (or equivalent).

[jmajors]

More feedback changes.

[zturner]

unittests/tools/lldb-server/tests/MessageObjects.cpp

168

Bear with me here, I'm determined to kill this stringstream ;-)

How about std::string hex_id = llvm::formatv("{0:x-2}", register_id);

x = lowercase hex.
- means don't print the `0x` prefix.
2 means print at least 2 digits.

[krytarowski]

In D32930#753916, @labath wrote:

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

In D32930#752843, @krytarowski wrote:

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

run the check-lldb-unit target.

OK, thanks!

[krytarowski]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

Can we go for raise(SIGTRAP)?

[labath]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

Doesn't work on windows (so it is not more portable than null dereference) and it has no payload (so you cannot use it for passing data to the test) :)

[zturner]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

Just use LLVM_BUILTIN_TRAP or a null pointer dereference, so it works everywhere.

[krytarowski]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

22

I'm for NULL pointer dereference.

volatile char *p = NULL;
*p = 'a';

or similar.

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.cpp

143

Since I'm returning copies of the pointer container in getter functions, I think I need shared, not unique.

190

I converted my pointer containers to Expected<unique_ptr<Type>>.
I noticed that ASSERT_* doesn't fail the test, it returns, so I need to make the functions in TestClient return Error or Expected<> objects and check them in the test body.

[jmajors]

Changed shared_ptr<Type> returns to Expected<unique_ptr<Type>>.
Some other error processing additions.

[zturner]

unittests/tools/lldb-server/tests/MessageObjects.cpp

27–28

How about std::unique_ptr<ProcessInfo> process_info(new ProcessInfo);

190

You'll want to use something like I've got in D33059 for checking the values of Expected<T>s and Errors. That's pending code review though. You can put the logic in the body as you suggested, but it's really cumbersome.

[labath]

A bunch more pedantic comments from me.

unittests/tools/lldb-server/inferior/thread_inferior.cpp

27

Could you add a (e.g. 1 second) sleep into the loop, to avoid the threads hogging the cpu.

unittests/tools/lldb-server/tests/MessageObjects.cpp

11

Do you still need these includes?

56

This should probably be a parsing error instead. Having the server tell us "my endianness is native" is not really helpful :).

93

If you're not too attached to curly braces, you can save some space here by dropping them. They have some value when the statement is complex, but for one-line bodies like this they just add clutter.

unittests/tools/lldb-server/tests/MessageObjects.h

11

System includes should go last. (If you delete the empty lines between the include blocks, clang-format will take care of the order for you).

21

our namespaces tend to be snake_cased for some reason. I'm not entirely happy with the "communication" in the name, as they test much more than just communication. I guess I'd call it llgs_test, even though it's not entirely correct (but it's shorter!), but I don't feel about that strongly.

26

If we're only going to be using this type for printing errors to the user (which seems to be the case now), we might as well use llvm::StringError (possibly with a custom factory function which would insert the "argument was invalid" stuff and such for brevity). What do you think?

I am asking that because I am not sure if all errors we encounter can be described as "parsing errors", and I wanted to avoid defining a bunch of additional error types, if we don't need it. If you see a use for that, then fine.

51

Using protected seems to hint that this class can be inherited from, which is generally a bad idea without providing a virtual destructor. (I'm not sure why would anyone want to inherit from this)

55

This type will not exist on windows. We can use lldb::pid_t instead. There is no lldb::uid_t and gid_t equivalent though (lldb seems to use simply uint32_t for these).

[labath]

unittests/tools/lldb-server/tests/MessageObjects.cpp

198

There's no guarantee lldb-server will send the register numbers in sequence. In the current implementation it expedites general purpose registers (which also happen to be the lowest numbered registers on x86 at least, but I am not sure if that is the case on all architectures). You would be better of looping through the key-value pairs and checking whether the key is a number (that's what the real client does).

200

llvm::formatv or something similar

unittests/tools/lldb-server/tests/TestClient.cpp

34

static const char g_local_host[] = ...; (static because the variable is file-local, [] avoids an extra indirection and makes the variable really const, the variable name is debatable, but it definitely shouldn't be all caps).

39

HostInfo::Initialize belongs to the SetUpTestCase (or possibly SetUp) of the fixture for these tests (that's how all other of our tests handle this).

125

return *process_info

179

We should return an error if we don't find the pc register.

[jmajors]

Moved asserts to the TEST(), where they'll actually do something.
Made TestClient members warn if their return values go unused.

[labath]

I think we're getting close, but I see a couple more issues here.

unittests/tools/lldb-server/tests/MessageObjects.cpp

25

I like how clean this ended up looking :)

83

What do you think hiding the formatv call inside make_parsing_error so we can write return make_parsing_error("JSON object at {0}", i);

unittests/tools/lldb-server/tests/MessageObjects.h

11

This still looks wrong. Did you run clang-format on the full patch (git clang-format origin/master should do the trick. you may need to set the clangFormat.binary git setting to point to a clang-format executable) ?

56

As far as I can tell, this StringRef points to the JSON object created in JThreadsInfo::Create, which is ephemeral to that function (I.e. it's a dangling reference). StringRefs are great for function arguments, but you have to be careful if you want to persist them. It's usually best to convert them back to a string at that point. Either std::string or llvm::SmallString<N> if you want to optimize (I guess N=16 would be a good value for thread names).

Same goes for other StringRefs persisted as member variables.

unittests/tools/lldb-server/tests/TestClient.cpp

63

This won't actually fail the test (i'm not sure whether you intended that or not). I think it would be better to bubble the error one more level and do the assert in the TEST. After zachary is done with D33059, we will have a nice way to assert on llvm::Error types. After I commit D33241, you can easily convert Status into llvm::Error.

Also the whole Error class is marked as nodiscard, so you won't need to annotate all functions with the macro explicitly.

116

This is a linux-ism. Other targets don't have the "pid == main thread id" concept. What is the semantics you intended for the thread_id = 0 case? If you wanted to resume the whole process (all threads) you can send vCont;c or just c. We also have the LLDB_INVALID_THREAD_ID symbolic constant to signify invalid thread.

149

The message argument could be a StringRef to give more flexibility to the callers. Here we don't have to worry about lifetime, as the function does not persist the message.

224

return log_file.str()

[tberghammer]

unittests/tools/lldb-server/tests/MessageObjects.h

42

The LLDB coding convention is to prefix member variables with "m_". Do we want to follow that one here or should we follow the LLVM one what is CapitalCase (you are following neither of them at the moment)?

53

"unsigned long" is only 32 bit on some systems but a register can be arbitrary large so the return value should be something more generic. This is true for every location you are working with registers.

64

Why do we need the unique_ptr here? Can it return llvm::Expected<ProcessInfo> instead? Same question for the other Create functions.

84

I think in an lldb-server test we should make as little assumption about lldb-server as possible. I am not aware of any packet where duplicated keys are allowed but if we want to rely on it I think we should add a test expectation verifying this as having repeated keys would mean lldb-server is buggy (what is exactly what we want to catch here)

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.cpp

11

Yes. I'm using a stringstream to convert integer register IDs to two nibble hex strings.

27–28

Java brainwashing. :)

unittests/tools/lldb-server/tests/MessageObjects.h

11

I ran clang-format -i *h *cpp. It reordered the includes.
I just ran it as a git subcommand, and it didn't change these lines.
I even tried scrambling the includes around, and that's the order it seems to want them in.

64

Since I don't build the JThreadsInfo, ProcessInfo, and StopReply members of TestClient in the constructor, I'd need a default constructor, which I hid to force use of Create(). Also, it allows me to have an uninitialized value for these members, so I can verify some things happen in the correct order.

unittests/tools/lldb-server/tests/TestClient.cpp

63

The false return/no discard combo causes the test to fail.
I didn't want to return an Error object, because it adds a lot of overhead.
If Zachary's assert change reduces this overhead, I can switch it.

116

I was using 0 so the caller didn't have to know what the main thread id was.

[jmajors]

More CL feedback.

[labath]

unittests/tools/lldb-server/tests/MessageObjects.h

11

Right, I think you're using the weird system clang-format present on google workstations. We can't use this one because it seems to hardcode google style, or something like that. Please try again using a clang-format built from upstream directly. When I did that on your patch, it put the includes in the correct order (and also fixed some other small style inconsistencies)

42

Agreed.

53

changing to uint64_t does not really solve the issue Tamas was pointing out, it possibly even makes it worse. The reason I did not bring it up until now is that long happens match the size of general purpose registers, which are the ones that we care about for the moment, which was kinda nice.

However, intel sse registers can be up to 512 bits wide, so we will need to have something more generic sooner or later. lldb has a RegisterValue class for this purpose, so we should probably use that. If it shows that it makes the code too clunky, we can add some accessor functions which assert that the size of register is e.g. sizeof(void*) and return a simple integer. They can then be used it cases where you know that the register must contain a pointer-sized value.

64

Avoiding a constructed-but-invalid object is certainly a worthwhile goal. Technically you can have that *and* avoid the unique_ptr here by having the test client store llvm::Optional<JThreadsInfo> as a member, which you then initialize after you get a proper jthreadsinfo response.

It would make the interface of this function nicer to use (you can write result->GetThreadInfoMap() instead of result.get()->GetThreadInfoMap()), but that is not too high on my list of priorities right now.

(actually, I am wondering now whether you really need the wrapper class, or the Create function could return the ThreadInfoMap object directly)

92

We can have both of these (and any other combination of arguments we want to by making this a template)

template<typename Args...>
llvm::Error make_parsing_error(llvm::StringRef format, Args &&... args) {
   std::string error = "Unable to parse " + formatv(format, std::forward<Args>(args)...).str();
  return make_error<StringError>(error, inconvertibleErrorCode());
}

should do the trick (after you fix the compile errors).

The interface of this function will then be the same as formatv, except it will return an error.

unittests/tools/lldb-server/tests/TestClient.cpp

63

With a macro like:

#define ASSERT_NO_ERROR(x)                                                     \
  if (::llvm::Error ASSERT_error = (x))                                        \
  FAIL() << #x                                                                 \
         << " failed\nerror: " << ::llvm::toString(::std::move(ASSERT_error))  \
         << "\n"

The call site would become:
ASSERT_NO_ERROR(client.StartDebugger());
and here you could just do return status.ToError();

PS: the macro here has the same interface as the one Zachary is moving, but has superior functionality (it prints the actual error message instead of just E was true and it even allows the user to stream additional messages into the macro). When the location has settled I'll send them a patch to improve it. It the mean time, I guess you can either have a local copy of the macro, or keep the bool thingy.

116

Right, so this won't work because on netbsd (I believe) the main thread will have tid = 1.

We could start special-casing the individual platforms here to get the right behaviour, but I am not sure the i-want-to-resume-only-the-main-thread-but-i-can't-be-bothered-to-look-it-up case is common enough for that -- If you don't have a thread ID handy, most of the time you will want to resume the whole process instead. So I think we should have two functions, one that resumes the whole process (which takes no arguments), and one that resumes only a single thread (and takes a mandatory argument).

(also the argument type should be lldb::tid_t)

[tberghammer]

unittests/tools/lldb-server/inferior/thread_inferior.cpp

23

You have to make this variable atomic (or add a mutex) to make it work. In the current implementation there is no guarantee that the new threads will ever see the updated value as (in theory) the compiler can optimize away the repeated read.

unittests/tools/lldb-server/tests/MessageObjects.cpp

55

(nit): You can use "ProcessInfo() = default;" in the header file (here and in every other empty constructor/destructor)

unittests/tools/lldb-server/tests/MessageObjects.h

34–43

A large part of these variables are never read by anybody. Do we want to keep them around just in case or should we remove them?

[krytarowski]

unittests/tools/lldb-server/tests/TestClient.cpp

116

This is correct, process has it's PID, and it has one or more LWPs (threads). The main thread is 1, and it's counting 2,3,4 for second, third, fourth etc.

Thread 0 is internally (and at least in ptrace(2)) reserved for "all process" events.

If I know correctly, the same model is in Solaris.

[jmajors]

unittests/tools/lldb-server/tests/MessageObjects.h

34–43

I figured I might as well parse the whole message at once, in case new tests need the pieces.

53

Rather than trying to test and convert them all, I just left them as strings. I only access one value (the PC), so I'm converting it as I need it.

[jmajors]

More feedback changes.

[labath]

Ok, so the comments below are basically a collection of nits. The only two major issues that still need addressing are:

• getting rid of the sleep in the startup code
• deciding on the naming of members

unittests/tools/lldb-server/tests/MessageObjects.cpp

24

if (!elements_or_error) return elements_or_error.takeError() is a bit shorter

28

This introduces a copy. It does not matter much for test code, but best be wary of that issue in general. You should do auto &elements = ...

unittests/tools/lldb-server/tests/MessageObjects.h

48

explicit not needed

51

StringRef

unittests/tools/lldb-server/tests/TestClient.cpp

23

Including iostream is banned.

Besides, I'm pretty sure you don't need it for what you are doing in this file.

61

Status has a format provider, so you can just drop the .AsCString() thingy here.

125

return llvm::None; Then you can drop the dummy optional object.

132

std::move(*creation) to avoid a copy

138

extra parens

217

std::move

223

static_cast<CFD*>

[jmajors]

CR feedback.

[jmajors]

Addressed more feedback.

[jmajors]

Added m_ prefix to member variables.
Converted lldb-server start to single thread.

[jmajors]

Format changes.

[labath]

Getting really close now. However, the debug trap issue is still not resolved. And we still have to figure out how to make sure the tests don't blow up on platforms that don't support debugging via lldb-server (i.e., anything except linux, android, netbsd). One option would be to just disable the inclusion of the whole subfolder at cmake level. Another is to stop gtest from running them by prefixing their names with DISABLED_. The usual solution for that is to define a helper macro like

#if defined(__linux__) || defined(__NetBSD__)
#define LLGS_TEST(x) x
#else
#define LLGS_TEST(x) DISABLED_ ## x
#endif

I'd tend towards the cmake option.

unittests/tools/lldb-server/inferior/thread_inferior.cpp

33

The debug trap issue is not resolved yet, as it evaluates to nothing on gcc. I believe the consensus was to go with null pointer dereference, at least for now. I am not sure how good of an idea is to use llvm headers in the test inferiors, but we can use LLVM_BUILTIN_TRAP macro for that purpose, at least until it starts causing us problems.

unittests/tools/lldb-server/tests/TestClient.cpp

241

this is very posix-y. Please use llvm::sys::fs::is_directory instead. (I may not have gotten the namespace right).

[jmajors]

Make inferior break more portable.
Restrict tests to running on Linux, BSD, or Android systems.

[jmajors]

Made directory test more portable.

[labath]

Ok, I think this makes a reasonable starting point for further work. We just need to tighten the condition on when to run these tests.

unittests/tools/CMakeLists.txt

2

This is not what I meant. The only targets (at least until we have debugserver support) that can realistically pass these tests are linux, android, and netbsd. The other targets (right now, I guess that would mean freebsd) don't even pretend to support debugging via lldb-server, so we should not fail their build because of that. Check for usages of CMAKE_SYSTEM_NAME to see how to discriminate those.

[beanz]

One small comment below. In general I agree with the thoughts here, and I think that this is a huge step forward for testing the debug server components.

I also agree with Zachary in principal that it would be nice to come up with lit-based test harnesses for more parts of LLDB, although I'm skeptical about whether or not that is actually the best way to test the debug server pieces. Either way, this is a huge step forward from what we have today, so we should go with it.

unittests/tools/CMakeLists.txt

2

Darwin pretends to support lldb-server in several places, it would be nice to be able to run these tests on Darwin if they work. One of my big goals for the future of testing on LLDB is to get to the point where the only differences in test coverage when running tests on different hosts is truly platform-specific code. Today we are nowhere near that.

Also, as Pavel pointed out in email, the lldb-server tests are also run against debugserver, so we need to make sure that still works too.

[labath]

Thanks for the support, @beanz.

unittests/tools/CMakeLists.txt

2

Which lldb-server support do you refer to here?

There is some llgs (debugging) support in lldb-server, but I have no idea what's the state of it -- it was added by Todd during his week of code as an "NFC" commit, and it hasn't been touched since. I'd like to avoid this keeping the build red if there is no intention of working on it.

The "platform" mode of lldb-server should work on darwin afaik, and we definitely want to be able to run it there. It's not what we are focusing on now though. We'd like to migrate the "debug" tests first (there are no "platform" tests), so the old ones can be removed.

In any case, I think of the apple exclusion part as a temporary thing, so we can check this in without breaking the build, we will pretty soon want to include it as well, so that we can run debugserver tests, at least. (At which point we will need a different way of disabling unsupported tests).

[beanz]

unittests/tools/CMakeLists.txt

2

I don't know the state of this stuff on Darwin either. I had spent a little time a few weeks ago trying to get lldb to use lldb-server on Darwin to see if I could answer that question, but I didn't get very far before I had to stop.

As long as the Apple exclusion is temporary I'm fine with this as-is. I'll see if I can free up some time this summer to look more closely at lldb-server on Darwin and figure out what state it is in.

[zturner]

In D32930#767820, @beanz wrote:

One small comment below. In general I agree with the thoughts here, and I think that this is a huge step forward for testing the debug server components.

I also agree with Zachary in principal that it would be nice to come up with lit-based test harnesses for more parts of LLDB, although I'm skeptical about whether or not that is actually the best way to test the debug server pieces. Either way, this is a huge step forward from what we have today, so we should go with it.

It would be nice if, at some point, we could move past "It's hard" and start getting into the details of what's hard about it. (Note this goes for LLDB client as well as lldb server). I see a lot of general hand-wavy comments about how conditionals are needed, or variables, etc, but that doesn't really do anything to convince me that it's hard. After all, we wrote a C++ compiler! And I'm pretty sure that the compiler-rt and sanitizer test suite is just as complicated as, if not more complicated than any hypothetical lldb test suite. And those have been solved.

What *would* help would be to ignore how difficult it may or may not be, and just take a couple of tests and re-write them in some DSL that you invent specifically for this purpose that is as concise as possible yet as expressive as you need, and we go from there. I did this with a couple of fairly hairy tests a few months ago and it didn't seem that bad to me.

The thing is, the set of people who are experts on the client side of LLDB and the set of people who are experts on the client side of LLVM/lit/etc are mostly disjoint, so nothing is ever going to happen without some sort of collaboration. For example, I'm more than willing to help out writing the lit bits of it, but I would need a specification of what the test language needs to look like to support all of the use cases. And someone else has to provide that since we want to get the design right. Even if implementing the language is hard, deciding what it needs to look like is supposed to be the easy part!

[labath]

I am going to commit this, so we can move forward with the remote testing aspect.

I'd like to reiterate that we are not planning to remove any existing tests until both remote and debugserver testing work. At that point, we'd like to start removing the existing tests, but we will bring that up again when the time comes. We can also certainly allow for some temporal flexibility if that happens to fall into someone release deadline or such.

unittests/tools/CMakeLists.txt

2

Yep, it's definitely temporary. Getting this thing working with debugserver is my #2 priority for this project (mainly because I expect it to be easy). Right now we are looking at the remote testing aspect of this, which is going to be more tricky, and without which we are dead in the water. If you have some thoughts or requirements on this aspect, now's the time to speak :). I am planning to make this generic enough so it can use both ssh and adb as a conduit, at which point I hope it can be used with iOS as well.

As for lldb-server on darwin, that will have to come from you guys, as I believe there is still a lot of work to be done to make it functional.

unittests/CMakeLists.txt

add_custom_command(
       TARGET ${test_name}
       POST_BUILD
       COMMAND "${CMAKE_COMMAND}" -E copy ${CMAKE_CURRENT_SOURCE_DIR}/Inputs/${INPUT} ${CMAKE_CURRENT_BINARY_DIR}/Inputs
       COMMENT "Copying ${INPUT} to binary directory.")
   endforeach()
 endfunction()
 
 add_subdirectory(Breakpoint)
 add_subdirectory(Core)
 add_subdirectory(Editline)
 add_subdirectory(Expression)
 add_subdirectory(Host)
 add_subdirectory(Interpreter)
 add_subdirectory(Language)
 add_subdirectory(ObjectFile)
 add_subdirectory(Platform)
 add_subdirectory(Process)
 add_subdirectory(ScriptInterpreter)
 add_subdirectory(Signals)
 add_subdirectory(Symbol)
 add_subdirectory(SymbolFile)
 add_subdirectory(Target)
+add_subdirectory(tools)
 add_subdirectory(UnwindAssembly)
 add_subdirectory(Utility)
 
 if(LLDB_CAN_USE_DEBUGSERVER)
   add_subdirectory(debugserver)
-endif()
+endif()
 

unittests/tools/CMakeLists.txt

+add_subdirectory(lldb-server)

unittests/tools/lldb-server/CMakeLists.txt

+function(add_lldb_test_executable test_name)
+  set(EXCLUDE_FROM_ALL ON)
+  add_llvm_executable(${test_name} NO_INSTALL_RPATH ${ARGN})
+  set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
+  set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
+endfunction()
+
+add_lldb_test_executable(thread_inferior inferior/thread_inferior.cpp)
+
+add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
+add_definitions(-DTHREAD_INFERIOR="${CMAKE_CURRENT_BINARY_DIR}/thread_inferior")
+add_subdirectory(tests)
+add_dependencies(LLDBServerTests thread_inferior)

unittests/tools/lldb-server/inferior/thread_inferior.cpp

+//===-- thread_inferior.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+#include <thread>
+#include <vector>
+
+#include "llvm/Support/Compiler.h"
+
+int main(int argc, char* argv[])
+{
+  int thread_count = 2;
+  if (argc > 1) {
+    thread_count = std::stoi(argv[1], nullptr, 10);
+  }
+
+  bool delay = true;
+  std::vector<std::thread> threads;
+  for (int i = 0; i < thread_count; i++) {
+    threads.push_back(std::thread([&delay]{while(delay);}));
+  }
+
+  LLVM_BUILTIN_DEBUGTRAP;
+  delay = false;
+  for (std::thread& t : threads) {
+    t.join();
+  }
+
+  return 0;
+}

unittests/tools/lldb-server/tests/CMakeLists.txt

+add_lldb_unittest(LLDBServerTests
+  TestClient.cpp
+  MessageObjects.cpp
+  ThreadIdsInJstopinfoTest.cpp
+
+  LINK_LIBS
+    lldbHost
+    lldbCore
+    lldbInterpreter
+    lldbTarget
+    lldbPluginPlatformLinux
+    lldbPluginProcessGDBRemote
+  LINK_COMPONENTS
+    Support
+  )

unittests/tools/lldb-server/tests/MessageObjects.h

+//===-- MessageObjects.h ----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "lldb/lldb-types.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/ADT/DenseMap.h"
+
+namespace CommunicationTests {
+  class ThreadInfo;
+  typedef llvm::DenseMap<unsigned long, ThreadInfo> ThreadInfoMap;
+  typedef llvm::DenseMap<unsigned long, unsigned long> ULongMap;
+
+class ProcessInfo {
+public:
+  explicit ProcessInfo(llvm::StringRef response);
+  pid_t GetPid() const;
+  llvm::support::endianness GetEndian() const;
+
+private:
+  pid_t pid;
+  pid_t parent_pid;
+  uid_t real_uid;
+  unsigned int real_gid;
+  uid_t effective_uid;
+  unsigned int effective_gid;
+  std::string triple;
+  llvm::SmallString<16> ostype;
+  llvm::support::endianness endian;
+  unsigned int ptrsize;
+};
+
+class ThreadInfo {
+public:
+  ThreadInfo();
+  explicit ThreadInfo(llvm::StringRef name, llvm::StringRef reason,
+                      const ULongMap& registers, unsigned int signal);
+
+  unsigned long ReadRegister(unsigned long register_id) const;
+
+private:
+  llvm::StringRef name;
+  llvm::StringRef reason;
+  ULongMap registers;
+  unsigned int signal;
+};
+
+class JThreadsInfo {
+public:
+  static std::shared_ptr<JThreadsInfo> Create(llvm::StringRef response,
+                                         llvm::support::endianness endian);
+
+  const ThreadInfoMap& GetThreadInfos() const;
+
+protected:
+  JThreadsInfo();
+
+private:
+  ThreadInfoMap thread_infos;
+};
+
+class StopReply {
+public:
+  static std::shared_ptr<StopReply> Create(llvm::StringRef response,
+                                           llvm::support::endianness endian);
+  const ULongMap& GetThreadPcs() const;
+
+protected:
+  StopReply();
+
+private:
+  void ParseResponse(llvm::StringRef response,
+                     llvm::support::endianness endian);
+  unsigned int signal;
+  lldb::tid_t thread;
+  llvm::StringRef name;
+  ULongMap thread_pcs;
+  ULongMap registers;
+  llvm::StringRef reason;
+};
+
+// Common functions for parsing packet data.
+llvm::StringMap<llvm::StringRef> SplitPairList(llvm::StringRef s);
+}

unittests/tools/lldb-server/tests/MessageObjects.cpp

+//===-- MessageObjects.cpp --------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <iomanip>
+#include <sstream>
+
+#include "lldb/Core/StructuredData.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include "gtest/gtest.h"
+#include "MessageObjects.h"
+
+using namespace lldb_private;
+using namespace llvm;
+using namespace llvm::support;
+namespace CommunicationTests {
+ProcessInfo::ProcessInfo(StringRef response) {
+  auto elements = SplitPairList(response);
+  elements["pid"].getAsInteger(16, pid);
+  elements["parent-pid"].getAsInteger(16, parent_pid);
+  elements["real-uid"].getAsInteger(16, real_uid);
+  elements["real-gid"].getAsInteger(16, real_gid);
+  elements["effective-uid"].getAsInteger(16, effective_uid);
+  elements["effective-gid"].getAsInteger(16, effective_gid);
+  triple = fromHex(elements["triple"]);
+  elements["ptrsize"].getAsInteger(10, ptrsize);
+  endian = llvm::StringSwitch<endianness>(elements["endian"])
+    .Case("little", support::little)
+    .Case("big", support::big)
+    .Default(support::native);
+}
+
+pid_t ProcessInfo::GetPid() const {
+  return pid;
+}
+
+endianness ProcessInfo::GetEndian() const {
+  return endian;
+}
+
+//====== ThreadInfo ============================================================
+ThreadInfo::ThreadInfo() {
+  // For map [] operators.
+}
+
+ThreadInfo::ThreadInfo(StringRef name, StringRef reason,
+                       const ULongMap& registers, unsigned int signal) :
+  name(name), reason(reason), registers(registers), signal(signal) {
+}
+
+unsigned long ThreadInfo::ReadRegister(unsigned long register_id) const {
+  return registers.lookup(register_id);
+}
+
+//====== JThreadsInfo ==========================================================
+std::shared_ptr<JThreadsInfo>
+JThreadsInfo::Create(StringRef response, endianness endian) {
+  std::shared_ptr<JThreadsInfo> jthreads_info =
+    std::shared_ptr<JThreadsInfo>(new JThreadsInfo);
+
+  StructuredData::ObjectSP json = StructuredData::ParseJSON(response);
+  StructuredData::Array* array = json->GetAsArray();
+  if (!array) {
+    return std::shared_ptr<JThreadsInfo>(nullptr);
+  }
+
+  for (size_t i = 0; i < array->GetSize(); i++) {
+    StructuredData::Dictionary* thread_info;
+    array->GetItemAtIndexAsDictionary(i, thread_info);
+    if (!thread_info) {
+      return std::shared_ptr<JThreadsInfo>(nullptr);
+    }
+
+    std::string name;
+    thread_info->GetValueForKeyAsString("name", name);
+    std::string reason;
+    thread_info->GetValueForKeyAsString("reason", reason);
+    unsigned long signal;
+    thread_info->GetValueForKeyAsInteger("signal", signal);
+    unsigned long tid;
+    thread_info->GetValueForKeyAsInteger("tid", tid);
+
+    StructuredData::Dictionary* register_dict;
+    thread_info->GetValueForKeyAsDictionary("registers", register_dict);
+    if (!register_dict) {
+      return std::shared_ptr<JThreadsInfo>(nullptr);
+    }
+
+    ULongMap registers;
+
+    auto keys_obj = register_dict->GetKeys();
+    auto keys = keys_obj->GetAsArray();
+    for (size_t i = 0; i < keys->GetSize(); i++) {
+      std::string key_str;
+      keys->GetItemAtIndexAsString(i, key_str);
+      std::string value_str;
+      register_dict->GetValueForKeyAsString(key_str, value_str);
+      StringRef key_str_ref(key_str);
+      StringRef value_str_ref(value_str);
+      unsigned int register_id;
+      if (key_str_ref.getAsInteger(10, register_id)) {
+        return std::shared_ptr<JThreadsInfo>(nullptr);
+      }
+      
+      unsigned long register_value;
+      if (value_str_ref.getAsInteger(16, register_value)) {
+        return std::shared_ptr<JThreadsInfo>(nullptr);
+      }
+      if (endian == little) {
+        sys::swapByteOrder(register_value);
+      }
+
+      registers[register_id] = register_value;
+    }
+
+    jthreads_info->thread_infos[tid] =
+      ThreadInfo(name, reason, registers, signal);
+  }
+
+  return jthreads_info;
+}
+
+const ThreadInfoMap& JThreadsInfo::GetThreadInfos() const {
+  return thread_infos;
+}
+
+JThreadsInfo::JThreadsInfo() {
+}
+
+//====== StopReply =============================================================
+StopReply::StopReply() {
+}
+
+const ULongMap& StopReply::GetThreadPcs() const {
+  return thread_pcs;
+}
+
+std::shared_ptr<StopReply> StopReply::Create(StringRef response,
+    llvm::support::endianness endian) {
+  auto stop_reply = std::shared_ptr<StopReply>(new StopReply());
+
+  auto elements = SplitPairList(response);
+  stop_reply->name = elements["name"];
+  stop_reply->reason = elements["reason"];
+
+  SmallVector<StringRef, 20> threads;
+  SmallVector<StringRef, 20> pcs;
+  elements["threads"].split(threads, ',');
+  elements["thread-pcs"].split(pcs, ',');
+  if (threads.size() != pcs.size()) {
+    return std::shared_ptr<StopReply>(nullptr);
+  }
+
+  for (unsigned int i = 0; i < threads.size(); i++) {
+    lldb::tid_t thread_id;
+    unsigned long pc;
+    threads[i].getAsInteger(16, thread_id);
+    pcs[i].getAsInteger(16, pc);
+    stop_reply->thread_pcs[thread_id] = pc;
+  }
+
+  unsigned int register_id = 0;
+  while (true) {
+    std::stringstream ss;
+    ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+    std::string hex_id = ss.str();
+    if (elements.find(hex_id) != elements.end()) {
+      unsigned long register_value;
+      if (elements[hex_id].getAsInteger(16, register_value)) {
+        return std::shared_ptr<StopReply>(nullptr);
+      }
+      if (endian == little) {
+        sys::swapByteOrder(register_value);
+      }
+
+      stop_reply->registers[register_id++] = register_value;
+    }
+    else {
+      break;
+    }
+  }
+
+  for (auto i = elements.begin(); i != elements.end(); i++) {
+    StringRef key = i->getKey();
+    StringRef val = i->getValue();
+    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);
+    }
+  }
+
+  return stop_reply;
+}
+
+//====== Globals ===============================================================
+StringMap<StringRef> SplitPairList(StringRef str) {
+  SmallVector<StringRef, 20> elements;
+  str.split(elements, ';');
+
+  StringMap<StringRef> pairs;
+  for (StringRef s : elements) {
+    pairs.insert(s.split(':'));
+  }
+
+  return pairs;
+}
+}

unittests/tools/lldb-server/tests/TestClient.h

+//===-- TestClient.h --------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <memory>
+#include <string>
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+
+#include "MessageObjects.h"
+
+namespace CommunicationTests {
+// TODO: Make the test client an abstract base class, with different children
+// for different types of connections: llgs v. debugserver
+class TestClient :
+  public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
+public:
+  TestClient(const std::string& test_name, const std::string& test_case_name);
+  virtual ~TestClient();
+  void StartDebugger();
+  void StopDebugger();
+  void SetInferior(llvm::ArrayRef<std::string> inferior_args);
+  void ListThreadsInStopReply();
+  void SetBreakpoint(unsigned long address);
+  void Continue(unsigned long thread_id = 0);
+  std::shared_ptr<ProcessInfo> GetProcessInfo();
+  std::shared_ptr<JThreadsInfo> GetJThreadsInfo();
+  std::shared_ptr<StopReply> GetLatestStopReply();
+  void SendMessage(const std::string& message);
+  void SendMessage(const std::string& message, std::string& response_string);
+  unsigned int GetPcRegisterId();
+
+private:
+  void GenerateConnectionAddress(std::string& address);
+  std::string GenerateLogFileName(const lldb_private::ArchSpec& arch) const;
+  std::string FormatFailedResult(const std::string& message,
+      lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
+      result);
+
+  std::shared_ptr<ProcessInfo> process_info;
+  std::shared_ptr<StopReply> stop_reply;
+  lldb_private::ProcessLaunchInfo server_process_info;
+  std::string test_name;
+  std::string test_case_name;
+  unsigned int pc_register;
+};
+}

unittests/tools/lldb-server/tests/TestClient.cpp

+//===-- TestClient.cpp ------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <cstdlib>
+#include <iostream>
+#include <sstream>
+#include <string>
+#include <future>
+
+#include "gtest/gtest.h"
+#include "TestClient.h"
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+#include "lldb/Host/posix/ProcessLauncherPosix.h"
+#include "lldb/Interpreter/Args.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+#include "llvm/ADT/StringExtras.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+namespace CommunicationTests {
+const char* LOCALHOST = "127.0.0.1";
+
+TestClient::TestClient(const std::string& test_name,
+                       const std::string& test_case_name)
+: test_name(test_name), test_case_name(test_case_name), pc_register(UINT_MAX) {
+  HostInfo::Initialize();
+}
+
+TestClient::~TestClient() {
+}
+
+void TestClient::StartDebugger() {
+  const ArchSpec& arch_spec = HostInfo::GetArchitecture();
+  Args args;
+  args.AppendArgument(LLDB_SERVER);
+  args.AppendArgument("gdbserver");
+  args.AppendArgument("--log-file=" + GenerateLogFileName(arch_spec));
+  args.AppendArgument("--log-channels=gdb-remote packets");
+  args.AppendArgument("--reverse-connect");
+  std::string connectionAddress;
+  GenerateConnectionAddress(connectionAddress);
+
+  args.AppendArgument(connectionAddress);
+  server_process_info.SetArchitecture(arch_spec);
+  server_process_info.SetArguments(args, true);
+  Error error = Host::LaunchProcess(server_process_info);
+  ASSERT_FALSE(error.Fail())
+    << "Failure to launch lldb server: " << error.AsCString();
+
+  sleep(5); // TODO: Sleep is bad. Can I wait for it to start?
+  SendAck(); // Send this as a handshake.
+}
+
+// TODO!!!!!!!!!!!!!!!!!!!!
+// Trace the quit command in the debugger, with a breakpoint at
+// SendPacketAndWaitForResponse() to see what command it sends to
+// the server to stop it.
+void TestClient::StopDebugger() {
+  Host::Kill(server_process_info.GetProcessID(), 15);
+}
+
+void TestClient::SetInferior(llvm::ArrayRef<std::string> inferior_args) {
+  std::stringstream command;
+  command << "A";
+  for (size_t i = 0; i < inferior_args.size(); i++) {
+    if (i > 0) command << ',';
+    std::string hex_encoded = toHex(inferior_args[i]);
+    command << hex_encoded.size() << ',' << i << ',' << hex_encoded;
+  }
+  
+  SendMessage(command.str());
+  SendMessage("qLaunchSuccess");
+  std::string response;
+  SendMessage("qProcessInfo", response);
+  process_info.reset(new ProcessInfo(response));
+}
+
+void TestClient::ListThreadsInStopReply() {
+  SendMessage("QListThreadsInStopReply");
+}
+
+void TestClient::SetBreakpoint(unsigned long address) {
+  std::stringstream command;
+  command << "Z0," << std::hex << address << ",1";
+  SendMessage(command.str());
+}
+
+void TestClient::Continue(unsigned long thread_id) {
+  ASSERT_TRUE(process_info.get())
+    << "Continue() called before process_info initialized.";
+
+  if (thread_id == 0) {
+    thread_id = process_info->GetPid();
+  }
+
+  std::stringstream message;
+  message << "vCont;c:" << std::hex << thread_id;
+  std::string response;
+  SendMessage(message.str(), response);
+  stop_reply = StopReply::Create(response, process_info->GetEndian());
+  ASSERT_TRUE(stop_reply.get()) << "Failure reading stop reply.";
+}
+
+std::shared_ptr<ProcessInfo> TestClient::GetProcessInfo() {
+  return process_info;
+}
+
+std::shared_ptr<JThreadsInfo> TestClient::GetJThreadsInfo() {
+  std::string response;
+  SendMessage("jThreadsInfo", response);
+  return JThreadsInfo::Create(response, process_info->GetEndian());
+}
+
+std::shared_ptr<StopReply> TestClient::GetLatestStopReply() {
+  return stop_reply;
+}
+
+void TestClient::SendMessage(const std::string& message) {
+  std::string dummy_string;
+  SendMessage(message, dummy_string);
+}
+
+void TestClient::SendMessage(const std::string& message,
+                             std::string& response_string) {
+  StringExtractorGDBRemote response;
+  PacketResult result = SendPacketAndWaitForResponse(message, response, false);
+  ASSERT_EQ(result, PacketResult::Success)
+    << FormatFailedResult(message, result);
+
+  response.GetEscapedBinaryData(response_string);
+  ASSERT_NE(response_string[0], 'E')
+    << "Error " << response_string << " while sending message: " << message;
+}
+
+unsigned int TestClient::GetPcRegisterId() {
+  if (pc_register != UINT_MAX) {
+    return pc_register;
+  }
+
+  for (unsigned int register_id = 0; ; register_id++) {
+    std::stringstream message;
+    message << "qRegisterInfo" << std::hex << register_id;
+    // This will throw if we scan all registers and never get the 
+    // expected result. Is that okay? Or is there a situation where
+    // no result is valid?
+    std::string response;
+    SendMessage(message.str(), response);
+    auto elements = SplitPairList(response);
+    if (elements["alt-name"] == "pc" || elements["generic"] == "pc") {
+      pc_register = register_id;
+      break;
+    }
+  }
+
+  return pc_register;
+}
+
+void TestClient::GenerateConnectionAddress(std::string& address) {
+  StartListenThread(LOCALHOST, 0);
+  auto connection = (ConnectionFileDescriptor*)GetConnection();
+  uint16_t listening_port = connection->GetListeningPort(UINT32_MAX);
+  ASSERT_GT(listening_port, 0) << "GetListeningPort failed.";
+
+  raw_string_ostream address_stream(address);
+  address_stream << LOCALHOST << ":" << listening_port;
+  address_stream.str();
+}
+
+std::string TestClient::GenerateLogFileName(const ArchSpec& arch) const {
+  std::string log_file_name;
+  raw_string_ostream log_file(log_file_name);
+  log_file << "lldb-test-traces/lldb-" << test_case_name << '-' << test_name
+           << '-' << arch.GetArchitectureName() << ".log";
+  log_file.str();
+  return log_file_name;
+}
+
+std::string TestClient::FormatFailedResult(const std::string& message,
+                                      PacketResult result) {
+  std::string formatted_error;
+  raw_string_ostream error_stream(formatted_error);
+  error_stream << "Failure sending message: " << message << " Result: ";
+    
+  switch (result) {
+  case PacketResult::ErrorSendFailed:
+    error_stream << "ErrorSendFailed";
+    break;
+  case PacketResult::ErrorSendAck:
+    error_stream << "ErrorSendAck";
+    break;
+  case PacketResult::ErrorReplyFailed:
+    error_stream << "ErrorReplyFailed";
+    break;
+  case PacketResult::ErrorReplyTimeout:
+    error_stream << "ErrorReplyTimeout";
+    break;
+  case PacketResult::ErrorReplyInvalid:
+    error_stream << "ErrorReplyInvalid";
+    break;
+  case PacketResult::ErrorReplyAck:
+    error_stream << "ErrorReplyAck";
+    break;
+  case PacketResult::ErrorDisconnected:
+    error_stream << "ErrorDisconnected";
+    break;
+  case PacketResult::ErrorNoSequenceLock:
+    error_stream << "ErrorNoSequenceLock";
+    break;
+  default:
+    error_stream << "Unknown Error";
+  }
+
+  error_stream.str();
+  return formatted_error;
+}
+}

unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp

+//===-- ThreadsInJstopinfoTest.cpp ------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+
+#include "gtest/gtest.h"
+#include "TestClient.h"
+
+using namespace CommunicationTests;
+
+TEST(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
+  std::vector<std::string> inferior_args;
+  // This inferior spawns N threads, then forces a break.
+  inferior_args.push_back(THREAD_INFERIOR);
+  inferior_args.push_back("4");
+
+  auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+
+  TestClient client(test_info->name(), test_info->test_case_name());
+  client.StartDebugger();
+  client.SetInferior(inferior_args);
+  client.ListThreadsInStopReply();
+  client.Continue();
+  unsigned int pc_reg = client.GetPcRegisterId();
+
+  auto jthreads_info = client.GetJThreadsInfo();
+  ASSERT_TRUE(jthreads_info.get()) << "Error reading JThreadInfo.";
+
+  auto stop_reply = client.GetLatestStopReply();
+  auto stop_reply_pcs = stop_reply->GetThreadPcs();
+  auto thread_infos = jthreads_info->GetThreadInfos();
+  ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
+    << "Thread count mismatch.";
+
+  for (auto stop_reply_pc : stop_reply_pcs) {
+    unsigned long tid = stop_reply_pc.first;
+    ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
+      << "Thread ID: " << tid << " not in JThreadsInfo.";
+    ASSERT_EQ(stop_reply_pcs[tid], thread_infos[tid].ReadRegister(pc_reg))
+      << "Mismatched PC for thread: " << tid;
+  }
+
+  client.StopDebugger();
+}