[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:10
+
+#include <memory>
+#include <string>
----------------
jmajors wrote:
> labath wrote:
> > 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) ?
> 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.
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)


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:52
+
+  unsigned long ReadRegister(unsigned long register_id) const;
+
----------------
tberghammer wrote:
> "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.
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.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:63
+ public:
+  static llvm::Expected<std::unique_ptr<JThreadsInfo>> Create(
+      llvm::StringRef response, llvm::support::endianness endian);
----------------
jmajors wrote:
> tberghammer wrote:
> > Why do we need the unique_ptr here? Can it return llvm::Expected<ProcessInfo> instead? Same question for the other Create functions.
> 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.
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)


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:91
+// Common functions for parsing packet data.
+llvm::Error make_parsing_error(const char *format, uint64_t number);
+llvm::Error make_parsing_error(llvm::StringRef parse_target);
----------------
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.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:41
+private:
+  std::string source;
+};
----------------
tberghammer wrote:
> 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)?
Agreed. 


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:62
+            .str();
+    GTEST_LOG_(ERROR) << error;
+    return false;
----------------
jmajors wrote:
> labath wrote:
> > 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.
> 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.
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.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:115
+
+  if (thread_id == 0) thread_id = process_info->GetPid();
+
----------------
jmajors wrote:
> labath wrote:
> > 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.
> I was using 0 so the caller didn't have to know what the main thread id was.
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)


https://reviews.llvm.org/D32930