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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The largest issue I see here is the use of exceptions, which are banned in llvm. We'll need to get rid of those before we start discussing anything else. This means we'll need to propagate errors manually, and we should design it in a way that it is easy to ASSERT on them. One option is to use llvm::Expected<T> for this, which is very good for this use, as it can actually contain a list of errors, which we can print out with a custom ASSERT_NO_ERROR macro. The usage would then be something like:

  Expected<Foo> FooOr = getFoo();
  ASSERT_NO_ERROR(FooOr);
  // use FooOr.get()

However this has a disadvantage of the error message not containing the expression that failed, so it could be better having the functions just return llvm::Error and have the real return be in a by-ref argument:

  Foo foo;
  ASSERT_NO_ERROR(getFoo(foo));

Other options are possible as well.. with a sufficiently crazy assert macro we could also achieve syntax like `Foo foo = ASSERT_NO_ERROR(getFoo());`, but that may be too confusing.

I also highlighted a couple of other cases of violating the llvm coding standards, but that will need more attention once this is sorted. Sorry :/



================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:9
+
+using namespace std;
+using namespace lldb_private;
----------------
using namespace std goes against llvm coding standards <http://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std>


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:16
+  pid = stoi(elements["pid"], nullptr, 16);
+  parent_pid = stoi(elements["parent-pid"], nullptr, 16);
+  real_uid = stoi(elements["real-uid"], nullptr, 16);
----------------
StringRef::getAsInteger


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:113
+  if (threads.size() != pcs.size()) {
+    throw TestClientException("Size mismatch between threads and thread-pcs.");
+  }
----------------
Exceptions are not allowed in llvm, so this will need to be written differently. To get around the constructor-cannot-fail problem, you can make the constructor private, and have a static factory function (`Create`?) instead, which returns Expected<T>. Then, all the operations that can fail are performed in the factory function (which can report failure), and the constructor is basically just used for initializing the fields).


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:84
+std::unordered_map<std::string, std::string> SplitPairList(const std::string& s);
+std::vector<std::string> SplitList(const std::string& s, char delimeter);
+std::pair<std::string, std::string> SplitPair(const std::string& s);
----------------
Delete and repace uses with StringRef::split


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:50
+  Args args;
+  args.SetCommandString(LLDB_SERVER);
+  args.AppendArgument("gdbserver");
----------------
Using SetCommandString is wrong here, as that will actually attempt to parse the path as a full command (which will fail be wrong if it contains spaces, quotes, etc.). Just append the program name as well.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:65
+
+  sleep(5); // TODO: Sleep is bad. Can I wait for it to start?
+  SendAck(); // Send this as a handshake.
----------------
Since you're using reverse-connect (which is good), you should be able to detect that the server is ready by the fact it has connected to you.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+  Host::Kill(server_process_info.GetProcessID(), 15);
+}
----------------
This is not portable.


https://reviews.llvm.org/D32930