[Lldb-commits] [PATCH] D108831: [lldb] [ABI/X86] Add pseudo-registers if missing

[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 think we've managed to come to something that looks mostly reasonable.



================
Comment at: lldb/source/Plugins/ABI/X86/ABIX86.cpp:51
+    std::vector<DynamicRegisterInfo::Register> &regs,
+    llvm::ArrayRef<uint32_t> base_reg_indices, RegisterMap reg_names,
+    uint32_t base_size, RegKind name_index, lldb::Encoding encoding,
----------------
const RegisterMap& -- it's expensive to copy


================
Comment at: lldb/source/Plugins/ABI/X86/ABIX86.cpp:59
+    DynamicRegisterInfo::Register &full_reg = regs[base_index];
+    const llvm::StringRef &subreg_name =
+        reg_names[full_reg.name.GetStringRef()][static_cast<int>(name_index)];
----------------
drop const & -- it's cheap to copy :)


================
Comment at: lldb/source/Plugins/ABI/X86/ABIX86.cpp:92-94
+  bool is64bit =
+      process_sp->GetTarget().GetArchitecture().GetAddressByteSize() == 8;
+  uint32_t gpr_base_size = is64bit ? 8 : 4;
----------------
how about you reverse this, so you set gpr_base_size first, and then use that to initialize is64bit?


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py:343
 
+        # test pseudo-registers
+        self.match("register read cx",
----------------
Maybe also do a `register read --all` and check that all (sub)registers are present and have the expected values (you can try using `self.filecheck` for that).


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

https://reviews.llvm.org/D108831