Page MenuHomePhabricator

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
labath added inline comments.May 15 2017, 5:44 AM
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 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
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 added inline comments.May 18 2017, 6:08 AM
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.

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)

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

jmajors marked 7 inline comments as done.May 18 2017, 2:10 PM
jmajors added inline comments.
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 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
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 added inline comments.May 19 2017, 5:02 AM
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 added inline comments.May 19 2017, 5:08 AM
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 marked 12 inline comments as done.May 19 2017, 4:19 PM
jmajors added inline comments.
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 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
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 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
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 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
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.

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

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

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

This revision was automatically updated to reflect the committed changes.