[Lldb-commits] [PATCH] D130342: [LLDB][RISCV] Add register stuff and make breakpoint work

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Can "and make breakpoint work" be made into its own patch? I'd much rather see "register read and write" and "software breakpoints" in their own changes (and please differentiate in commit title/messages between hardware break and software break).

(otherwise the content seems fine just some comments on names and formatting)



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_riscv64.h:49
+  {                                                                            \
+#reg, #alt, 8, GPR_OFFSET(gpr_##reg - gpr_first), lldb::eEncodingUint,     \
+        lldb::eFormatHex, GPR64_KIND(gpr_##reg, generic_kind), nullptr,        \
----------------
clang-format will align these to the left. So I suggest adding "// clang-format off" to disable that and go with the "clang-format on" below. Then indent them as if they were normal code.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_riscv64.h:130
+
+    // clang-format on
+};
----------------
I check the arm64 file and it also had an orphaned "on". I fixed that in https://github.com/llvm/llvm-project/commit/552dccf311c6134c6c895328602172821e3efaed.

See comment above.


================
Comment at: lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h:20
+  gpr_pc = gpr_first,
+  gpr_x1,
+  gpr_x2,
----------------
We want to keep the "riscv" tag here. It provides a sort of namespace for the enumerator otherwise overlapping names in different anonymous enums is an error.

https://godbolt.org/z/1W8P1z83T

E.g. if arm64 just used "gpr_x0" we'd get an error, so it uses "gpr_x0_arm64" to prevent that.


================
Comment at: lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h:94
 
-  k_first_vcr_riscv,
-  vcr_v0_riscv = k_first_vcr_riscv,
-  vcr_v1_riscv,
-  vcr_v2_riscv,
-  vcr_v3_riscv,
-  vcr_v4_riscv,
-  vcr_v5_riscv,
-  vcr_v6_riscv,
-  vcr_v7_riscv,
-  vcr_v8_riscv,
-  vcr_v9_riscv,
-  vcr_v10_riscv,
-  vcr_v11_riscv,
-  vcr_v12_riscv,
-  vcr_v13_riscv,
-  vcr_v14_riscv,
-  vcr_v15_riscv,
-  vcr_v16_riscv,
-  vcr_v17_riscv,
-  vcr_v18_riscv,
-  vcr_v19_riscv,
-  vcr_v20_riscv,
-  vcr_v21_riscv,
-  vcr_v22_riscv,
-  vcr_v23_riscv,
-  vcr_v24_riscv,
-  vcr_v25_riscv,
-  vcr_v26_riscv,
-  vcr_v27_riscv,
-  vcr_v28_riscv,
-  vcr_v29_riscv,
-  vcr_v30_riscv,
-  vcr_v31_riscv,
-  vcr_vstart_riscv,
-  vcr_vxsat_riscv,
-  vcr_vxrm_riscv,
-  vcr_vcsr_riscv,
-  vcr_vl_riscv,
-  vcr_vtype_riscv,
-  vcr_vlenb_riscv,
-  k_last_vcr_riscv = vcr_vlenb_riscv,
-
-  k_num_registers_riscv,
-  k_num_gpr_registers_riscv = k_last_gpr_riscv - k_first_gpr_riscv + 1,
-  k_num_fpr_registers_riscv = k_last_fpr_riscv - k_first_fpr_riscv + 1,
-  k_num_vcr_registers_riscv = k_last_vcr_riscv - k_first_vcr_riscv + 1,
+  k_num_registers
 };
----------------
Why did the vector registers get removed?


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

https://reviews.llvm.org/D130342