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

122–123 ↗

(On Diff #98044)

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

133 ↗

(On Diff #98044)

That container lacks a necessary constructor.

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

82–87 ↗

(On Diff #98044)

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

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

65 ↗

(On Diff #98044)

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.

70 ↗

(On Diff #98044)

Is there a portable way of stopping?

[jmajors]

Made changes based on feedback.

[zturner]

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

21 ↗

(On Diff #98673)

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

83–88 ↗

(On Diff #98044)

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

64 ↗

(On Diff #98673)

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

69 ↗

(On Diff #98673)

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

21 ↗

(On Diff #98673)

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

98 ↗

(On Diff #98675)

Do you need to check for an error here?

105 ↗

(On Diff #98675)

And here

[krytarowski]

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

200 ↗

(On Diff #98675)

Throws exceptions.

[jmajors]

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

200 ↗

(On Diff #98675)

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

83–88 ↗

(On Diff #98044)

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.

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

76–80 ↗

(On Diff #98044)

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

1 ↗

(On Diff #98691)

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

70 ↗

(On Diff #98044)

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

21 ↗

(On Diff #98673)

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

61–63 ↗

(On Diff #98044)

As of r302875 this is no longer true.

83–88 ↗

(On Diff #98044)

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

83–88 ↗

(On Diff #98044)

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

21 ↗

(On Diff #98673)

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

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

83–88 ↗

(On Diff #98044)

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

70 ↗

(On Diff #98044)

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

25–30 ↗

(On Diff #98826)

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?

109 ↗

(On Diff #98826)

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.

142 ↗

(On Diff #98826)

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.

159–160 ↗

(On Diff #98826)

Is Error checking needed on these two calls?

167 ↗

(On Diff #98826)

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

189 ↗

(On Diff #98826)

Is error checking needed here?

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

105–108 ↗

(On Diff #98826)

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

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

155–156 ↗

(On Diff #98826)

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

[krytarowski]

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

21 ↗

(On Diff #98673)

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

33 ↗

(On Diff #98826)

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

21 ↗

(On Diff #98673)

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

91 ↗

(On Diff #98826)

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

142 ↗

(On Diff #98826)

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.

189 ↗

(On Diff #98826)

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

65 ↗

(On Diff #98044)

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.

70 ↗

(On Diff #98044)

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

25–30 ↗

(On Diff #98826)

I changed it to a Create with tests for success.

142 ↗

(On Diff #98826)

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

167 ↗

(On Diff #98826)

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

167 ↗

(On Diff #98826)

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

21 ↗

(On Diff #98673)

Can we go for raise(SIGTRAP)?

[labath]

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

21 ↗

(On Diff #98673)

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

21 ↗

(On Diff #98673)

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

[krytarowski]

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

21 ↗

(On Diff #98673)

I'm for NULL pointer dereference.

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

or similar.

[jmajors]

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

142 ↗

(On Diff #98826)

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

189 ↗

(On Diff #98826)

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

26–27 ↗

(On Diff #98869)

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

189 ↗

(On Diff #98826)

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

26 ↗

(On Diff #98869)

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

10 ↗

(On Diff #98869)

Do you still need these includes?

55 ↗

(On Diff #98869)

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

92 ↗

(On Diff #98869)

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

10 ↗

(On Diff #98869)

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).

20 ↗

(On Diff #98869)

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.

25 ↗

(On Diff #98869)

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.

50 ↗

(On Diff #98869)

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)

54 ↗

(On Diff #98869)

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

197 ↗

(On Diff #98869)

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).

199 ↗

(On Diff #98869)

llvm::formatv or something similar

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

33 ↗

(On Diff #98869)

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).

38 ↗

(On Diff #98869)

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).

124 ↗

(On Diff #98869)

return *process_info

178 ↗

(On Diff #98869)

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

24 ↗

(On Diff #99365)

I like how clean this ended up looking :)

82 ↗

(On Diff #99365)

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

10 ↗

(On Diff #99365)

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) ?

55 ↗

(On Diff #99365)

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

62 ↗

(On Diff #99365)

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.

115 ↗

(On Diff #99365)

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.

148 ↗

(On Diff #99365)

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.

223 ↗

(On Diff #99365)

return log_file.str()

[tberghammer]

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

52 ↗

(On Diff #99365)

"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.

63 ↗

(On Diff #99365)

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

41 ↗

(On Diff #98869)

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)?

83 ↗

(On Diff #98044)

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

10 ↗

(On Diff #98869)

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

26–27 ↗

(On Diff #98869)

Java brainwashing. :)

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

10 ↗

(On Diff #99365)

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.

63 ↗

(On Diff #99365)

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

62 ↗

(On Diff #99365)

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.

115 ↗

(On Diff #99365)

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

10 ↗

(On Diff #99365)

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)

52 ↗

(On Diff #99365)

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.

63 ↗

(On Diff #99365)

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)

41 ↗

(On Diff #98869)

Agreed.

91 ↗

(On Diff #99491)

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

62 ↗

(On Diff #99365)

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.

115 ↗

(On Diff #99365)

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

22 ↗

(On Diff #99491)

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

54 ↗

(On Diff #99491)

(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

33–42 ↗

(On Diff #99491)

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

115 ↗

(On Diff #99365)

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

52 ↗

(On Diff #99365)

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.

33–42 ↗

(On Diff #99491)

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

[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

23 ↗

(On Diff #99644)

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

27 ↗

(On Diff #99644)

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

47 ↗

(On Diff #99644)

explicit not needed

50 ↗

(On Diff #99644)

StringRef

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

22 ↗

(On Diff #99644)

Including iostream is banned.

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

60 ↗

(On Diff #99644)

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

124 ↗

(On Diff #99644)

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

131 ↗

(On Diff #99644)

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

137 ↗

(On Diff #99644)

extra parens

216 ↗

(On Diff #99644)

std::move

222 ↗

(On Diff #99644)

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

32 ↗

(On Diff #100178)

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

240 ↗

(On Diff #100178)

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

1 ↗

(On Diff #100288)

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

1 ↗

(On Diff #100288)

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

1 ↗

(On Diff #100288)

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

1 ↗

(On Diff #100288)

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

1 ↗

(On Diff #100288)

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.

lldb/trunk/unittests/CMakeLists.txt

 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()
 

lldb/trunk/unittests/tools/CMakeLists.txt

+if(CMAKE_SYSTEM_NAME MATCHES "Android|Linux|NetBSD")
+  add_subdirectory(lldb-server)
+endif()

lldb/trunk/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)

lldb/trunk/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 <atomic>
+#include <chrono>
+#include <string>
+#include <thread>
+#include <vector>
+
+int main(int argc, char* argv[]) {
+  int thread_count = 2;
+  if (argc > 1) {
+    thread_count = std::stoi(argv[1], nullptr, 10);
+  }
+
+  std::atomic<bool> delay(true);
+  std::vector<std::thread> threads;
+  for (int i = 0; i < thread_count; i++) {
+    threads.push_back(std::thread([&delay] {
+      while (delay.load())
+        std::this_thread::sleep_for(std::chrono::seconds(1));
+    }));
+  }
+
+  // Cause a break.
+  volatile char *p = NULL;
+  *p = 'a';
+
+  delay.store(false);
+  for (std::thread& t : threads) {
+    t.join();
+  }
+
+  return 0;
+}

lldb/trunk/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
+  )

lldb/trunk/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 "lldb/lldb-types.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <string>
+
+namespace llgs_tests {
+class ThreadInfo;
+typedef llvm::DenseMap<uint64_t, ThreadInfo> ThreadInfoMap;
+typedef llvm::DenseMap<uint64_t, uint64_t> U64Map;
+typedef llvm::DenseMap<unsigned int, std::string> RegisterMap;
+
+class ProcessInfo {
+public:
+  static llvm::Expected<ProcessInfo> Create(llvm::StringRef response);
+  lldb::pid_t GetPid() const;
+  llvm::support::endianness GetEndian() const;
+
+private:
+  ProcessInfo() = default;
+  lldb::pid_t m_pid;
+  lldb::pid_t m_parent_pid;
+  uint32_t m_real_uid;
+  uint32_t m_real_gid;
+  uint32_t m_effective_uid;
+  uint32_t m_effective_gid;
+  std::string m_triple;
+  llvm::SmallString<16> m_ostype;
+  llvm::support::endianness m_endian;
+  unsigned int m_ptrsize;
+};
+
+class ThreadInfo {
+public:
+  ThreadInfo() = default;
+  ThreadInfo(llvm::StringRef name, llvm::StringRef reason,
+             const RegisterMap &registers, unsigned int signal);
+
+  llvm::StringRef ReadRegister(unsigned int register_id) const;
+  bool ReadRegisterAsUint64(unsigned int register_id, uint64_t &value) const;
+
+private:
+  std::string m_name;
+  std::string m_reason;
+  RegisterMap m_registers;
+  unsigned int m_signal;
+};
+
+class JThreadsInfo {
+public:
+  static llvm::Expected<JThreadsInfo> Create(llvm::StringRef response,
+                                             llvm::support::endianness endian);
+
+  const ThreadInfoMap &GetThreadInfos() const;
+
+private:
+  JThreadsInfo() = default;
+  ThreadInfoMap m_thread_infos;
+};
+
+class StopReply {
+public:
+  static llvm::Expected<StopReply> Create(llvm::StringRef response,
+                                          llvm::support::endianness endian);
+  const U64Map &GetThreadPcs() const;
+
+private:
+  StopReply() = default;
+  void ParseResponse(llvm::StringRef response,
+                     llvm::support::endianness endian);
+  unsigned int m_signal;
+  lldb::tid_t m_thread;
+  std::string m_name;
+  U64Map m_thread_pcs;
+  RegisterMap m_registers;
+  std::string m_reason;
+};
+
+// Common functions for parsing packet data.
+llvm::Expected<llvm::StringMap<llvm::StringRef>>
+SplitPairList(llvm::StringRef caller, llvm::StringRef s);
+
+template <typename... Args>
+llvm::Error make_parsing_error(llvm::StringRef format, Args &&... args) {
+  std::string error =
+      "Unable to parse " +
+      llvm::formatv(format.data(), std::forward<Args>(args)...).str();
+  return llvm::make_error<llvm::StringError>(error,
+                                             llvm::inconvertibleErrorCode());
+}
+} // namespace llgs_tests

lldb/trunk/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 "MessageObjects.h"
+#include "lldb/Core/StructuredData.h"
+#include "llvm/ADT/StringExtras.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace llvm;
+using namespace llvm::support;
+namespace llgs_tests {
+
+Expected<ProcessInfo> ProcessInfo::Create(StringRef response) {
+  ProcessInfo process_info;
+  auto elements_or_error = SplitPairList("ProcessInfo", response);
+  if (!elements_or_error)
+    return elements_or_error.takeError();
+
+  auto &elements = *elements_or_error;
+  if (elements["pid"].getAsInteger(16, process_info.m_pid))
+    return make_parsing_error("ProcessInfo: pid");
+  if (elements["parent-pid"].getAsInteger(16, process_info.m_parent_pid))
+    return make_parsing_error("ProcessInfo: parent-pid");
+  if (elements["real-uid"].getAsInteger(16, process_info.m_real_uid))
+    return make_parsing_error("ProcessInfo: real-uid");
+  if (elements["real-gid"].getAsInteger(16, process_info.m_real_gid))
+    return make_parsing_error("ProcessInfo: real-uid");
+  if (elements["effective-uid"].getAsInteger(16, process_info.m_effective_uid))
+    return make_parsing_error("ProcessInfo: effective-uid");
+  if (elements["effective-gid"].getAsInteger(16, process_info.m_effective_gid))
+    return make_parsing_error("ProcessInfo: effective-gid");
+  if (elements["ptrsize"].getAsInteger(10, process_info.m_ptrsize))
+    return make_parsing_error("ProcessInfo: ptrsize");
+
+  process_info.m_triple = fromHex(elements["triple"]);
+  StringRef endian_str = elements["endian"];
+  if (endian_str == "little")
+    process_info.m_endian = support::little;
+  else if (endian_str == "big")
+    process_info.m_endian = support::big;
+  else
+    return make_parsing_error("ProcessInfo: endian");
+
+  return process_info;
+}
+
+lldb::pid_t ProcessInfo::GetPid() const { return m_pid; }
+
+endianness ProcessInfo::GetEndian() const { return m_endian; }
+
+//====== ThreadInfo ============================================================
+ThreadInfo::ThreadInfo(StringRef name, StringRef reason,
+                       const RegisterMap &registers, unsigned int signal)
+    : m_name(name.str()), m_reason(reason.str()), m_registers(registers),
+      m_signal(signal) {}
+
+StringRef ThreadInfo::ReadRegister(unsigned int register_id) const {
+  return m_registers.lookup(register_id);
+}
+
+bool ThreadInfo::ReadRegisterAsUint64(unsigned int register_id,
+                                      uint64_t &value) const {
+  StringRef value_str(m_registers.lookup(register_id));
+  if (value_str.getAsInteger(16, value)) {
+    GTEST_LOG_(ERROR)
+        << formatv("ThreadInfo: Unable to parse register value at {0}.",
+                   register_id)
+               .str();
+    return false;
+  }
+
+  sys::swapByteOrder(value);
+  return true;
+}
+
+//====== JThreadsInfo ==========================================================
+Expected<JThreadsInfo> JThreadsInfo::Create(StringRef response,
+                                            endianness endian) {
+  JThreadsInfo jthreads_info;
+
+  StructuredData::ObjectSP json = StructuredData::ParseJSON(response);
+  StructuredData::Array *array = json->GetAsArray();
+  if (!array)
+    return make_parsing_error("JThreadsInfo: JSON array");
+
+  for (size_t i = 0; i < array->GetSize(); i++) {
+    StructuredData::Dictionary *thread_info;
+    array->GetItemAtIndexAsDictionary(i, thread_info);
+    if (!thread_info)
+      return make_parsing_error("JThreadsInfo: JSON obj at {0}", i);
+
+    StringRef name, reason;
+    thread_info->GetValueForKeyAsString("name", name);
+    thread_info->GetValueForKeyAsString("reason", reason);
+    uint64_t signal;
+    thread_info->GetValueForKeyAsInteger("signal", signal);
+    uint64_t tid;
+    thread_info->GetValueForKeyAsInteger("tid", tid);
+
+    StructuredData::Dictionary *register_dict;
+    thread_info->GetValueForKeyAsDictionary("registers", register_dict);
+    if (!register_dict)
+      return make_parsing_error("JThreadsInfo: registers JSON obj");
+
+    RegisterMap registers;
+
+    auto keys_obj = register_dict->GetKeys();
+    auto keys = keys_obj->GetAsArray();
+    for (size_t i = 0; i < keys->GetSize(); i++) {
+      StringRef key_str, value_str;
+      keys->GetItemAtIndexAsString(i, key_str);
+      register_dict->GetValueForKeyAsString(key_str, value_str);
+      unsigned int register_id;
+      if (key_str.getAsInteger(10, register_id))
+        return make_parsing_error("JThreadsInfo: register key[{0}]", i);
+
+      registers[register_id] = value_str.str();
+    }
+
+    jthreads_info.m_thread_infos[tid] =
+        ThreadInfo(name, reason, registers, signal);
+  }
+
+  return jthreads_info;
+}
+
+const ThreadInfoMap &JThreadsInfo::GetThreadInfos() const {
+  return m_thread_infos;
+}
+
+//====== StopReply =============================================================
+const U64Map &StopReply::GetThreadPcs() const { return m_thread_pcs; }
+
+Expected<StopReply> StopReply::Create(StringRef response,
+                                      llvm::support::endianness endian) {
+  StopReply stop_reply;
+
+  auto elements_or_error = SplitPairList("StopReply", response);
+  if (auto split_error = elements_or_error.takeError()) {
+    return std::move(split_error);
+  }
+
+  auto elements = *elements_or_error;
+  stop_reply.m_name = elements["name"];
+  stop_reply.m_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 make_parsing_error("StopReply: thread/PC count mismatch");
+
+  for (size_t i = 0; i < threads.size(); i++) {
+    lldb::tid_t thread_id;
+    uint64_t pc;
+    if (threads[i].getAsInteger(16, thread_id))
+      return make_parsing_error("StopReply: thread ID at [{0}].", i);
+    if (pcs[i].getAsInteger(16, pc))
+      return make_parsing_error("StopReply: thread PC at [{0}].", i);
+
+    stop_reply.m_thread_pcs[thread_id] = pc;
+  }
+
+  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") {
+      if (val.getAsInteger(16, stop_reply.m_thread))
+        return make_parsing_error("StopReply: thread id");
+      if (key.substr(1, 2).getAsInteger(16, stop_reply.m_signal))
+        return make_parsing_error("StopReply: stop signal");
+    } else if (key.size() == 2) {
+      unsigned int reg;
+      if (!key.getAsInteger(16, reg)) {
+        stop_reply.m_registers[reg] = val.str();
+      }
+    }
+  }
+
+  return stop_reply;
+}
+
+//====== Globals ===============================================================
+Expected<StringMap<StringRef>> SplitPairList(StringRef caller, StringRef str) {
+  SmallVector<StringRef, 20> elements;
+  str.split(elements, ';');
+
+  StringMap<StringRef> pairs;
+  for (StringRef s : elements) {
+    std::pair<StringRef, StringRef> pair = s.split(':');
+    if (pairs.count(pair.first))
+      return make_parsing_error("{0}: Duplicate Key: {1}", caller, pair.first);
+
+    pairs.insert(s.split(':'));
+  }
+
+  return pairs;
+}
+} // namespace llgs_tests

lldb/trunk/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 "MessageObjects.h"
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+#include "llvm/ADT/Optional.h"
+#include <memory>
+#include <string>
+
+namespace llgs_tests {
+// 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:
+  static void Initialize();
+  TestClient(const std::string &test_name, const std::string &test_case_name);
+  virtual ~TestClient();
+  LLVM_NODISCARD bool StartDebugger();
+  LLVM_NODISCARD bool StopDebugger();
+  LLVM_NODISCARD bool SetInferior(llvm::ArrayRef<std::string> inferior_args);
+  LLVM_NODISCARD bool ListThreadsInStopReply();
+  LLVM_NODISCARD bool SetBreakpoint(unsigned long address);
+  LLVM_NODISCARD bool ContinueAll();
+  LLVM_NODISCARD bool ContinueThread(unsigned long thread_id);
+  const ProcessInfo &GetProcessInfo();
+  llvm::Optional<JThreadsInfo> GetJThreadsInfo();
+  const StopReply &GetLatestStopReply();
+  LLVM_NODISCARD bool SendMessage(llvm::StringRef message);
+  LLVM_NODISCARD bool SendMessage(llvm::StringRef message,
+                                  std::string &response_string);
+  LLVM_NODISCARD bool SendMessage(llvm::StringRef message,
+                                  std::string &response_string,
+                                  PacketResult expected_result);
+  unsigned int GetPcRegisterId();
+
+private:
+  LLVM_NODISCARD bool Continue(llvm::StringRef message);
+  LLVM_NODISCARD bool 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);
+
+  llvm::Optional<ProcessInfo> m_process_info;
+  llvm::Optional<StopReply> m_stop_reply;
+  lldb_private::ProcessLaunchInfo m_server_process_info;
+  std::string m_test_name;
+  std::string m_test_case_name;
+  unsigned int m_pc_register;
+};
+} // namespace llgs_tests

lldb/trunk/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 "TestClient.h"
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Host/common/TCPSocket.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"
+#include "gtest/gtest.h"
+#include <cstdlib>
+#include <future>
+#include <sstream>
+#include <string>
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+namespace llgs_tests {
+void TestClient::Initialize() { HostInfo::Initialize(); }
+
+TestClient::TestClient(const std::string &test_name,
+                       const std::string &test_case_name)
+    : m_test_name(test_name), m_test_case_name(test_case_name),
+      m_pc_register(UINT_MAX) {}
+
+TestClient::~TestClient() {}
+
+bool TestClient::StartDebugger() {
+  const ArchSpec &arch_spec = HostInfo::GetArchitecture();
+  Args args;
+  args.AppendArgument(LLDB_SERVER);
+  args.AppendArgument("gdbserver");
+  args.AppendArgument("--log-channels=gdb-remote packets");
+  args.AppendArgument("--reverse-connect");
+  std::string log_file_name = GenerateLogFileName(arch_spec);
+  if (log_file_name.size()) {
+    args.AppendArgument("--log-file=" + log_file_name);
+  }
+
+  Status error;
+  TCPSocket listen_socket(true, false);
+  error = listen_socket.Listen("127.0.0.1:0", 5);
+  if (error.Fail()) {
+    GTEST_LOG_(ERROR) << "Unable to open listen socket.";
+    return false;
+  }
+
+  char connect_remote_address[64];
+  snprintf(connect_remote_address, sizeof(connect_remote_address),
+           "localhost:%u", listen_socket.GetLocalPortNumber());
+
+  args.AppendArgument(connect_remote_address);
+
+  m_server_process_info.SetArchitecture(arch_spec);
+  m_server_process_info.SetArguments(args, true);
+  Status status = Host::LaunchProcess(m_server_process_info);
+  if (status.Fail()) {
+    GTEST_LOG_(ERROR)
+        << formatv("Failure to launch lldb server: {0}.", status).str();
+    return false;
+  }
+
+  char connect_remote_uri[64];
+  snprintf(connect_remote_uri, sizeof(connect_remote_uri), "connect://%s",
+           connect_remote_address);
+  Socket *accept_socket;
+  listen_socket.Accept(accept_socket);
+  SetConnection(new ConnectionFileDescriptor(accept_socket));
+
+  SendAck(); // Send this as a handshake.
+  return true;
+}
+
+bool TestClient::StopDebugger() {
+  std::string response;
+  return SendMessage("k", response, PacketResult::ErrorDisconnected);
+}
+
+bool 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;
+  }
+
+  if (!SendMessage(command.str()))
+    return false;
+  if (!SendMessage("qLaunchSuccess"))
+    return false;
+  std::string response;
+  if (!SendMessage("qProcessInfo", response))
+    return false;
+  auto create_or_error = ProcessInfo::Create(response);
+  if (auto create_error = create_or_error.takeError()) {
+    GTEST_LOG_(ERROR) << toString(std::move(create_error));
+    return false;
+  }
+
+  m_process_info = *create_or_error;
+  return true;
+}
+
+bool TestClient::ListThreadsInStopReply() {
+  return SendMessage("QListThreadsInStopReply");
+}
+
+bool TestClient::SetBreakpoint(unsigned long address) {
+  std::stringstream command;
+  command << "Z0," << std::hex << address << ",1";
+  return SendMessage(command.str());
+}
+
+bool TestClient::ContinueAll() { return Continue("vCont;c"); }
+
+bool TestClient::ContinueThread(unsigned long thread_id) {
+  return Continue(formatv("vCont;c:{0:x-}", thread_id).str());
+}
+
+const ProcessInfo &TestClient::GetProcessInfo() { return *m_process_info; }
+
+Optional<JThreadsInfo> TestClient::GetJThreadsInfo() {
+  std::string response;
+  if (!SendMessage("jThreadsInfo", response))
+    return llvm::None;
+  auto creation = JThreadsInfo::Create(response, m_process_info->GetEndian());
+  if (auto create_error = creation.takeError()) {
+    GTEST_LOG_(ERROR) << toString(std::move(create_error));
+    return llvm::None;
+  }
+
+  return std::move(*creation);
+}
+
+const StopReply &TestClient::GetLatestStopReply() {
+  return m_stop_reply.getValue();
+}
+
+bool TestClient::SendMessage(StringRef message) {
+  std::string dummy_string;
+  return SendMessage(message, dummy_string);
+}
+
+bool TestClient::SendMessage(StringRef message, std::string &response_string) {
+  if (!SendMessage(message, response_string, PacketResult::Success))
+    return false;
+  else if (response_string[0] == 'E') {
+    GTEST_LOG_(ERROR) << "Error " << response_string
+                      << " while sending message: " << message.str();
+    return false;
+  }
+
+  return true;
+}
+
+bool TestClient::SendMessage(StringRef message, std::string &response_string,
+                             PacketResult expected_result) {
+  StringExtractorGDBRemote response;
+  GTEST_LOG_(INFO) << "Send Packet: " << message.str();
+  PacketResult result = SendPacketAndWaitForResponse(message, response, false);
+  response.GetEscapedBinaryData(response_string);
+  GTEST_LOG_(INFO) << "Read Packet: " << response_string;
+  if (result != expected_result) {
+    GTEST_LOG_(ERROR) << FormatFailedResult(message, result);
+    return false;
+  }
+
+  return true;
+}
+
+unsigned int TestClient::GetPcRegisterId() {
+  if (m_pc_register != UINT_MAX)
+    return m_pc_register;
+
+  for (unsigned int register_id = 0;; register_id++) {
+    std::string message = formatv("qRegisterInfo{0:x-}", register_id).str();
+    std::string response;
+    if (!SendMessage(message, response)) {
+      GTEST_LOG_(ERROR) << "Unable to query register ID for PC register.";
+      return UINT_MAX;
+    }
+
+    auto elements_or_error = SplitPairList("GetPcRegisterId", response);
+    if (auto split_error = elements_or_error.takeError()) {
+      GTEST_LOG_(ERROR) << "GetPcRegisterId: Error splitting response: "
+                        << response;
+      return UINT_MAX;
+    }
+
+    auto elements = *elements_or_error;
+    if (elements["alt-name"] == "pc" || elements["generic"] == "pc") {
+      m_pc_register = register_id;
+      break;
+    }
+  }
+
+  return m_pc_register;
+}
+
+bool TestClient::Continue(StringRef message) {
+  if (!m_process_info.hasValue()) {
+    GTEST_LOG_(ERROR) << "Continue() called before m_process_info initialized.";
+    return false;
+  }
+
+  std::string response;
+  if (!SendMessage(message, response))
+    return false;
+  auto creation = StopReply::Create(response, m_process_info->GetEndian());
+  if (auto create_error = creation.takeError()) {
+    GTEST_LOG_(ERROR) << toString(std::move(create_error));
+    return false;
+  }
+
+  m_stop_reply = std::move(*creation);
+  return true;
+}
+
+std::string TestClient::GenerateLogFileName(const ArchSpec &arch) const {
+  char *log_directory = getenv("LOG_FILE_DIRECTORY");
+  if (!log_directory)
+    return "";
+
+  if (!llvm::sys::fs::is_directory(log_directory)) {
+    GTEST_LOG_(WARNING) << "Cannot access log directory: " << log_directory;
+    return "";
+  }
+
+  std::string log_file_name;
+  raw_string_ostream log_file(log_file_name);
+  log_file << log_directory << "/lldb-" << m_test_case_name << '-'
+           << m_test_name << '-' << arch.GetArchitectureName() << ".log";
+  return log_file.str();
+}
+
+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;
+}
+} // namespace llgs_tests

lldb/trunk/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 "TestClient.h"
+#include "gtest/gtest.h"
+#include <string>
+
+using namespace llgs_tests;
+
+class ThreadsInJstopinfoTest : public ::testing::Test {
+protected:
+  virtual void SetUp() { TestClient::Initialize(); }
+};
+
+TEST_F(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());
+  ASSERT_TRUE(client.StartDebugger());
+  ASSERT_TRUE(client.SetInferior(inferior_args));
+  ASSERT_TRUE(client.ListThreadsInStopReply());
+  ASSERT_TRUE(client.ContinueAll());
+  unsigned int pc_reg = client.GetPcRegisterId();
+  ASSERT_NE(pc_reg, UINT_MAX);
+
+  auto jthreads_info = client.GetJThreadsInfo();
+  ASSERT_TRUE(jthreads_info);
+
+  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.";
+    uint64_t pc_value;
+    ASSERT_TRUE(thread_infos[tid].ReadRegisterAsUint64(pc_reg, pc_value))
+        << "Failure reading ThreadInfo register " << pc_reg;
+    ASSERT_EQ(stop_reply_pcs[tid], pc_value)
+        << "Mismatched PC for thread: " << tid;
+  }
+
+  ASSERT_TRUE(client.StopDebugger());
+}