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

[Jason Majors via Phabricator via lldb-commits]

jmajors marked 43 inline comments as done.
jmajors added a comment.

Every comment should either be marked done, with an accompanying code change, or have a reply comment.



================
Comment at: unittests/CMakeLists.txt:57
 add_subdirectory(Breakpoint)
+add_subdirectory(tools)
 add_subdirectory(Core)
----------------
takuto.ikuta wrote:
> better to sort alphabetically?
> Using tools instead of Tools intentionally?
I'm following the naming and case of the source directories.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:61-63
+    string name;
+    thread_info->GetValueForKeyAsString("name", name);
+    string reason;
----------------
zturner wrote:
> `StringRef name, reason;`
There is no GetValueForKeyAsStringRef(). I need a std::string to populate.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:77-79
+      string key_str;
+      keys->GetItemAtIndexAsString(i, key_str);
+      string value_str;
----------------
zturner wrote:
> `StringRef key_str, value_str;`
These need to be strings for the same reason as above.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:83-88
+      if (endian == LITTLE) {
+        registers[register_id] = SwitchEndian(value_str);
+      }
+      else {
+        registers[register_id] = stoul(value_str, nullptr, 16);
+      }
----------------
zturner wrote:
> This code block is unnecessary.
> 
> ```
> unsigned long X;
> if (!value_str.getAsInteger(X))
>   return some error;
> llvm::support::endian::write(&registers[register_id], X, endian);
> ```
> 
> By using llvm endianness functions you can just delete the `SwitchEndian()` function entirely, as it's not needed.
These endian functions don't work here. getAsInteger() assumes the number is in human reading order (i.e. big endian). So it's converting the little endian string as if it's big, then converting that little endian long to another little endian long, which does nothing.
I need something that reverses the string first.

What do you think about adding a version of StringRef::getAsInteger that accepts an endianness parameter?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:122-123
+  while (true) {
+    stringstream ss;
+    ss << hex << setw(2) << setfill('0') << register_id;
+    string hex_id = ss.str();
----------------
zturner wrote:
> Use `llvm::raw_string_ostream` or `raw_svector_ostream` instead of `stringstream`.
I don't see anything in either class or their ancestors that will format the number as a two nibble hex value.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:133
+
+  for (auto i = elements.begin(); i != elements.end(); i++) {
+    if (i->first[0] == 'T' && i->first.substr(3, 6) == "thread") {
----------------
takuto.ikuta wrote:
> Why not range based for?
That container lacks a necessary constructor.


================
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);
----------------
tberghammer wrote:
> Does these have to be global? Can we make them local to the .cc file (anonymous namespace or file static variable)?
I use one of them in the TestClient.cpp file. I could make the other local.


================
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);
----------------
zturner wrote:
> tberghammer wrote:
> > What if the key isn't unique?
> Return an `llvm::StringMap<StringRef>` here.  Also the argument should be a `StringRef`.
I was working on the assumption that the keys in lldb response packets would be unique, and that it would map a key to a list of values if it needed to have a one:many relationship. Are there any that have duplicate keys?


================
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.
----------------
labath wrote:
> 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.
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.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+  Host::Kill(server_process_info.GetProcessID(), 15);
+}
----------------
labath wrote:
> This is not portable.
Is there a portable way of stopping?


https://reviews.llvm.org/D32930