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

[Tamas Berghammer via Phabricator via lldb-commits]

tberghammer added a comment.

I few high level comments after a quick read through.



================
Comment at: unittests/tools/lldb-server/.gitignore:1
+thread_inferior
----------------
Why do we need this? I would expect cmake to *not* put anything into my source directory when doing an out-of-source build so I can have multiple build directory (e.g. for different targets) at the same time.


================
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+  asm volatile ("int3");
+  delay = false;
----------------
krytarowski wrote:
> int3 is specific to x86.
> 
> Some instructions to emit software breakpoint in other architectures:
> https://github.com/NetBSD/src/commit/c215d1b7d6c1385720b27fd45189b5dea6d6e4aa
> 
> Also there is a support for threads, this is not as portable as it could be. The simplest example could be without them, and threads in debuggee could be tested in dedicated regression tests. This will help out to sort bugs with threads from other features.
I think there should be a compiler intrinsic available as well.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:17
+
+class ProcessInfo {
+public:
----------------
Can we somehow reuse the logic in GDBRemoteCommunicationClient for parsing the packets (possibly after factoring out some of it into new standalone functions)? I think it would remove code duplication as well as increase the coverage provided by these tests.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:82-87
+// Common functions for parsing packet data.
+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);
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);
----------------
Does these have to be global? Can we make them local to the .cc file (anonymous namespace or file static variable)?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:83
+// Common functions for parsing packet data.
+std::unordered_map<std::string, std::string> SplitPairList(const std::string& s);
+std::vector<std::string> SplitList(const std::string& s, char delimeter);
----------------
What if the key isn't unique?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:86-87
+std::pair<std::string, std::string> SplitPair(const std::string& s);
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);
+}
----------------
I would hope we have functions in LLVM/LLDB doing these sort of things (might have to be changed slightly to make them easily accessible)


https://reviews.llvm.org/D32930