[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[Jessica Clarke via Phabricator via lldb-commits]

jrtc27 added inline comments.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:118
+
+  // Previous frames pc is in ra
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
----------------



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:141
+
+  // The previous frames pc is stored in ra.
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
----------------



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:142
+  // The previous frames pc is stored in ra.
+  row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
+
----------------
And SP is just.. the same? Surely the default unwind plan is to load SP and RA via FP? You won't get very far with this.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:167-168
+          .Cases("x22", "x23", "x24", "x25", "x26", "x27", true)
+          .Cases("f8", "f9", "f18", "f19", "f20", "f21", IsHardFloatProcess())
+          .Cases("f22", "f23", "f24", "f25", "f26", "f27", IsHardFloatProcess())
+          .Default(false);
----------------
What about if I have the D extension but only use LP64F as the ABI? Does it matter that this says f9 is callee-saved but in reality only the low 32 bits are?


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:16
+class ABISysV_riscv : public lldb_private::MCBasedABI {
+  bool isRV64;
+
----------------
IsRV64?


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:70-71
+    // Ensure addresses are smaller than XLEN bits wide. Calls can use the least
+    // significant bit to store auxiliary information, so no strict check is
+    // done for alignment.
+    if (!isRV64)
----------------
2 and 3 mod 4 are nevertheless invalid if you don't have the C extension and will take an alignment fault


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:72-73
+    // done for alignment.
+    if (!isRV64)
+      return (pc <= UINT32_MAX);
+    return (pc <= UINT64_MAX);
----------------
Are addresses reliably zero-extended or can you ever get cases where they're sign-extended from a valid 32-bit address to a 64-bit address?


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1152
+      triple.getArch() == llvm::Triple::riscv64)
+    features_str += "+a,+c,+d,+f,+m";
+
----------------
This will override whatever the ELF says in its attributes section. This might make sense as a default for if you don't have a binary and are just poking at memory, but it makes things worse when you do, the common case that need to work as best as we can manage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732