[Lldb-commits] [PATCH] D82853: [LLDB] Support custom expedited register set in gdb-remote

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

I like this a lot. LGTM with some small fixes inline.



================
Comment at: lldb/source/Host/common/NativeRegisterContext.cpp:428-434
+    static const uint32_t k_expedited_registers[] = {
+        LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_SP, LLDB_REGNUM_GENERIC_FP,
+        LLDB_REGNUM_GENERIC_RA, LLDB_INVALID_REGNUM};
+
+    std::vector<uint32_t> expedited_reg_nums;
+    for (const uint32_t *generic_reg_p = k_expedited_registers;
+         *generic_reg_p != LLDB_INVALID_REGNUM; ++generic_reg_p) {
----------------
Remove LLDB_INVALID_REGNUM from the list. Then iterate as `for(uint32_t gen_reg: k_expedited_registers)`.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:513-515
+  if (expedited_regs.empty())
+    return llvm::make_error<llvm::StringError>("failed to get registers",
+                                               llvm::inconvertibleErrorCode());
----------------
Let's change the result type to `Optional<Object>` and `return None` here -- now that we support customizing the expedited registers, I think it's reasonable to allow a register context to say it does not want to expedite any registers. (The current for of the code already kind of supports that, but the use of Expected makes it sound like that is an erroneous state.)


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:798
 
-    for (const uint32_t *reg_num_p = reg_set_p->registers;
-         *reg_num_p != LLDB_INVALID_REGNUM; ++reg_num_p) {
+  if (!expedited_regs.empty()) {
+    for (auto &reg_num : expedited_regs) {
----------------
I don't think this if is needed now -- we could remove it and save one level of indentation.


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

https://reviews.llvm.org/D82853