This is an archive of the discontinued LLVM Phabricator instance.

New framework for lldb client-server communication tests.
ClosedPublic

Authored by jmajors on May 5 2017, 6:09 PM.

Details

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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jmajors added inline comments.May 11 2017, 1:21 PM
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 updated this revision to Diff 98673.May 11 2017, 1:23 PM
jmajors marked an inline comment as done.

Made changes based on feedback.

zturner added inline comments.May 11 2017, 2:07 PM
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 marked 2 inline comments as done.May 11 2017, 2:18 PM
jmajors added inline comments.
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 updated this revision to Diff 98675.May 11 2017, 2:18 PM

Feedback changes.

zturner added inline comments.May 11 2017, 2:21 PM
unittests/tools/lldb-server/tests/MessageObjects.cpp
99

Do you need to check for an error here?

106

And here

krytarowski added inline comments.May 11 2017, 2:40 PM
unittests/tools/lldb-server/tests/MessageObjects.cpp
201

Throws exceptions.

jmajors marked 5 inline comments as done.May 11 2017, 3:34 PM
jmajors added inline comments.
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 updated this revision to Diff 98691.May 11 2017, 3:35 PM
jmajors marked an inline comment as done.

More feedback changes.

krytarowski added inline comments.May 11 2017, 3:53 PM
unittests/tools/lldb-server/inferior/thread_inferior.cpp
2

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

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

jmajors marked an inline comment as done.May 11 2017, 4:38 PM

I cleaned up the includes.

jmajors updated this revision to Diff 98702.May 11 2017, 4:38 PM

Cleaned up the includes.

krytarowski added inline comments.May 11 2017, 7:12 PM
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?

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

krytarowski added inline comments.May 11 2017, 7:56 PM
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.

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

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 added inline comments.May 11 2017, 11:14 PM
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 added inline comments.May 12 2017, 8:01 AM
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 marked 17 inline comments as done.May 12 2017, 1:13 PM

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 updated this revision to Diff 98826.May 12 2017, 1:16 PM
jmajors marked an inline comment as done.

Feedback changes.

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 added inline comments.May 12 2017, 1:53 PM
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 added inline comments.May 12 2017, 1:56 PM
unittests/tools/lldb-server/tests/MessageObjects.h
34

Is there an option to use gid_t?

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

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 marked 6 inline comments as done.May 12 2017, 3:53 PM
jmajors added inline comments.
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 updated this revision to Diff 98858.May 12 2017, 3:54 PM
jmajors marked an inline comment as done.

More feedback changes.

zturner added inline comments.May 12 2017, 4:05 PM
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.

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

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

run the check-lldb-unit target.

OK, thanks!

krytarowski added inline comments.May 12 2017, 4:11 PM
unittests/tools/lldb-server/inferior/thread_inferior.cpp
22

Can we go for raise(SIGTRAP)?

labath added inline comments.May 12 2017, 4:17 PM
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 added inline comments.May 12 2017, 4:23 PM
unittests/tools/lldb-server/inferior/thread_inferior.cpp
22

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

krytarowski added inline comments.May 12 2017, 4:30 PM
unittests/tools/lldb-server/inferior/thread_inferior.cpp
22

I'm for NULL pointer dereference.

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

or similar.

jmajors marked 7 inline comments as done.May 12 2017, 7:33 PM
jmajors added inline comments.
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 updated this revision to Diff 98869.May 12 2017, 7:44 PM
jmajors marked 2 inline comments as done.

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

zturner added inline comments.May 12 2017, 8:17 PM
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.

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 added inline comments.May 15 2017, 5:44 AM
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 updated this revision to Diff 99365.May 17 2017, 4:58 PM
jmajors marked 15 inline comments as done.

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

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 added inline comments.May 18 2017, 6:08 AM
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 marked 7 inline comments as done.May 18 2017, 2:10 PM
jmajors added inline comments.
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 updated this revision to Diff 99491.May 18 2017, 2:11 PM
jmajors marked an inline comment as done.

More CL feedback.

emaste added a subscriber: emaste.May 18 2017, 7:50 PM
labath added inline comments.May 19 2017, 2:59 AM
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 added inline comments.May 19 2017, 5:02 AM
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 added inline comments.May 19 2017, 5:08 AM
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 marked 12 inline comments as done.May 19 2017, 4:19 PM
jmajors added inline comments.
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 updated this revision to Diff 99644.May 19 2017, 4:20 PM
jmajors marked an inline comment as done.

More feedback changes.

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 updated this revision to Diff 99788.May 22 2017, 10:56 AM
jmajors marked 11 inline comments as done.

CR feedback.

Addressed more feedback.

jmajors updated this revision to Diff 100151.May 24 2017, 1:01 PM

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

jmajors updated this revision to Diff 100178.May 24 2017, 3:46 PM

Format changes.

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 updated this revision to Diff 100287.May 25 2017, 1:21 PM

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

jmajors updated this revision to Diff 100288.May 25 2017, 1:27 PM
jmajors marked an inline comment as done.

Made directory test more portable.

jmajors marked an inline comment as done.May 25 2017, 1:27 PM
labath accepted this revision.May 26 2017, 2:43 AM

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.

This revision is now accepted and ready to land.May 26 2017, 2:43 AM
beanz accepted this revision.May 30 2017, 8:54 AM
beanz added a subscriber: 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.

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 added inline comments.May 31 2017, 10:22 AM
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.

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 added a comment.Jun 6 2017, 6:39 AM

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.

This revision was automatically updated to reflect the committed changes.