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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:145
+  return false;
+}
+
----------------
ted wrote:
> 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
> 
Ok cool, as long as one of us knows what these do :)

The other way to kinda chaos engineer this is to take your host's target, make a function fail and see what parts of the test suite fail. Then that can tell you what to test manually for riscv's version of the same functions.


================
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,";
   }
----------------
ted wrote:
> 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.
Fair enough, I see that +all does not in fact work for risc-v llvm-objdump which backs that up. I guess you'd have to pass through some kind of object attributes to detect some of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101