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

[Ted Woodward via Phabricator via lldb-commits]

ted marked 13 inline comments as done.
ted added inline comments.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:107
+static size_t word_size = 4U;
+static size_t reg_size = word_size;
+
----------------
DavidSpickett wrote:
> 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.
I copied this from the ARC ABI plugin.

word_size and reg_size are used in computations, in PrepareTrivialCall and SetReturnValueObject.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:145
+  return false;
+}
+
----------------
DavidSpickett wrote:
> 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.
The first one is used for calling functions via JIT. The second is used for calling functions via the IR Interpreter. I didn't want to enable JIT, so I took the Hexagon implementation (Hexagon doesn't support JIT in lldb, but can call functions with the IR interpreter) and reworked it for RISC-V.

Here's a function call on riscv64:
(lldb) re r pc
      pc = 0x00000000000106b2 factrv64`main + 28 at factorial.c:32:8  factrv64`main + 28 at factorial.c:32:8
(lldb) p factorial(3)
(int) 6



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:310
+
+    raw_value >>= 32;
+    reg_info =
----------------
DavidSpickett wrote:
> 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.
Yes - on rv64, a 128 bit scalar can be passed in ARG1 and ARG2. Get the next chunk of data from data and write that.


================
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,";
   }
----------------
DavidSpickett wrote:
> 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.
While I like the "turn on the latest" philosophy in general, for RISC-V we don't want to do that. It's modular architecture means features can be turned on and off when a core is designed, so one core might have +d (floating point double), while an newer core might not have any floating point at all. I'm inclined to leave the features as they are now, with a and m turned on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101