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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
