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> ®s) {
----------------
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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
