[Lldb-commits] [PATCH] D74217: Add target.xml support for qXfer request.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is looking pretty good overall. I just have a bunch of stylistic remarks...



================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/registers-target-xml-reading/main.cpp:1
+#include <cstdint>
+
----------------
I don't think the choice of inferior really matters here. This code could just be like `int main(){}`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:849
   response.PutCString(";qXfer:auxv:read+");
+  response.PutCString(";qXfer:features:read+");
   response.PutCString(";qXfer:libraries-svr4:read+");
----------------
You can put this outside the `#ifdef`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:380-459
+static std::string GetEncodingNameOrEmpty(const RegisterInfo *reg_info) {
+  switch (reg_info->encoding) {
+  case eEncodingUint:
+    return "uint";
+  case eEncodingSint:
+    return "sint";
+  case eEncodingIEEE754:
----------------
These should return a `llvm::StringRef` to avoid copies, and take the argument as a reference to convey non-nullness.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2786-2910
+  if (object == "features" && annex == "target.xml") {
+    // Make sure we have a valid process.
+    if (!m_debugged_process_up ||
+        (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "No process available");
+    }
----------------
Please move this into a separate function


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2788-2792
+    if (!m_debugged_process_up ||
+        (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "No process available");
+    }
----------------
This check could be factored to the front of the function (the svr4 branch does currently not have it, but it most likely should).


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2841-2843
+      if (!encoding.empty()) {
+        response.Printf("encoding=\"%s\" ", encoding.c_str());
+      }
----------------
We generally don't put curly braces around statements which fit on a single line.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2871-2894
+      if (reg_info->value_regs &&
+          reg_info->value_regs[0] != LLDB_INVALID_REGNUM) {
+        response.PutCString("value_regnums=\"");
+        int i = 0;
+        for (const uint32_t *reg_num = reg_info->value_regs;
+             *reg_num != LLDB_INVALID_REGNUM; ++reg_num, ++i) {
+          if (i > 0)
----------------
Make a utility function (lambda?) for this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74217/new/

https://reviews.llvm.org/D74217