[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

>From what little I know about the ABI this looks ok. Consider adding a riscv expert to check those bits for you.

Testing wise riscv is in a strange place without a working lldb-server or a buildbot that could run such a thing (and qemu-user doesn't emulate ptrace calls). Getting the test suite to run against qemu instead is probably more effort than it's worth assuming lldb-server is not that far off working. If that's true then I'd be ok accepting this, it's quite self contained in any case and we have all that instruction emulation code for single stepping already.

If someone could get lldb-server running on qemu-system and test against that (just locally) that would be a great improvement and likely flush out many problems with a lot of the more niche methods in these classes.



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:11
+
+// C Includes
+// C++ Includes
----------------
In general most of these include comments you can remove, but this one in particular because there are no C includes.

We don't have a rule that states you comment each block, clang-format may re-order them but that's it.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:107
+static size_t word_size = 4U;
+static size_t reg_size = word_size;
+
----------------
What's the reason to do this this way? It seems like adding an extra argument to the constructor of ABISysV_riscv  would do the same thing unless someone is calling the constructor directly but that's what CreateInstance tries to prevent.

I see that mips has 2 ABI plugins, and MacOS on x86 has i386 and x86_64. The former I don't think has massive differences between 32 and 64 but the latter does.

So I agree with sharing the ABI here between riscv32 and riscv64 just with the way it's implemented.

If it's because you use an array later, either make it a vector or better an llvm::SmallVector which for rv32 will likely just use stack space for 4 byte stuff.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:145
+  return false;
+}
+
----------------
Looking at the comments in lldb/include/lldb/Target/ABI.h I'm not sure which of these should be implemented. I think this one is what most plugins provide.

One way to figure this out is to figure out what actually needs this. Return false from both and try a bunch of things to see if it fails, run an expression, step in and out etc.

I'd be more comfortable having one not implemented if we know how the other one gets used.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:217
+      while (byte_index < end) {
+        reg_value[byte_index++] = *(value++);
+        --size;
----------------
If, as I think, this and the bit below are just memcpy and memset, just use those and make it obvious.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:221
+
+      while (byte_index < reg_size) {
+        reg_value[byte_index++] = 0;
----------------
Add a comment here like "// Pad if the type's size is smaller than the register.".


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:310
+
+    raw_value >>= 32;
+    reg_info =
----------------
I see this came from Arc which appears to be 32 bit, so this needs to change for rv64?

If it doesn't, add comments above to note where rv64 would exit before getting here.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:392
+
+  if (sizeof(uint32_t) >= byte_size) {
+    // Read a0 to get the arg
----------------
I'm not sure, but perhaps `switch (byte_size)` removing one level of if/else would make this whole thing more clear.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:408
+      return return_valobj_sp;
+    } else {
+      // Create the ValueObjectSP here and return
----------------
No need for else after a return.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:462
+  case ArchSpec::eRISCV_float_abi_soft:
+    return_valobj_sp = GetValObjFromIntRegs(thread, reg_ctx, machine,
+                                            type_flags, byte_size);
----------------
    return GetValObjFromIntRegs(thread, reg_ctx, machine, type_flags, byte_size);


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:467
+  case ArchSpec::eRISCV_float_abi_single:
+    if (4 >= byte_size)
+      use_fp_regs = true;
----------------
In general don't use this order for comparisons, it's harder to understand as a reader. `byte_size <= 4` is easier.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:482
+  }
+  if (use_fp_regs){
+    uint64_t raw_value;
----------------
Blank line between this and the switch above it.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:492
+      return return_valobj_sp;
+  } else {
+      return_valobj_sp = GetValObjFromIntRegs(thread, reg_ctx, machine,
----------------
use_fp_regs is a bool so `if (use_fp_regs)` the else is always taken if false, so you don't need else here. Simply:
```
if ( ...) {
}

// implicit else
return ...;
```

Unless you want to do:
```
if (...) {
   return_valobj_sp = ...
} else
   return_valobj_sp = ...

return return_valobj_sp;
```

Either is fine.



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:540
+    if (compiler_type.IsFloatingPointType(float_count, is_complex) &&
+        1 == float_count && !is_complex) {
+      const uint32_t arch_fp_flags = arch.GetFlags() &
----------------
Once more for emphasis, please reverse the order of comparisons like this.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:701
+
+  void ABISysV_riscv::AugmentRegisterInfo(
+    std::vector<lldb_private::DynamicRegisterInfo::Register> &regs) {
----------------
Please clang format all the changes. The easiest way is to use https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting, ` git diff -U0 --no-color --relative HEAD^ | clang-format-diff.py -p1 -i`.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:705
+
+  for (auto it : llvm::enumerate(regs)) {
+    lldb_private::DynamicRegisterInfo::Register &info = it.value();
----------------
Looks like plain iteration would do the job, no use for the index here.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:79-80
+    uint32_t arch_flags = arch.GetFlags();
+    if (!(arch_flags & lldb_private::ArchSpec::eRISCV_rvc))
+      if (pc & 2)
+        return false;
----------------
Combine this into one if.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1554
+    // Turn them on by default now, since everyone seems to use them
+    features_str += "+a,+m,";
   }
----------------
You might want to take the lead from AArch64 here:
```
// If any AArch64 variant, enable latest ISA with all extensions.
```
If "+all" doesn't already work for riscv then you don't have to go and make that work right now.

But in general we decided that much like llvm-objdump, we'll try to disassemble any possible encoding. If the user happens to point the disassembler at garbage that looks like a fancy extension on a cpu from 20 years ago, that's on them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101