jrtc27 added inline comments.
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:118
+
+ // Previous frames pc is in ra
+ row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
----------------
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:141
+
+ // The previous frames pc is stored in ra.
+ row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
----------------
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:142
+ // The previous frames pc is stored in ra.
+ row->SetRegisterLocationToRegister(pc_reg_num, ra_reg_num, true);
+
----------------
And SP is just.. the same? Surely the default unwind plan is to load SP and RA
via FP? You won't get very far with this.
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:167-168
+ .Cases("x22", "x23", "x24", "x25", "x26", "x27", true)
+ .Cases("f8", "f9", "f18", "f19", "f20", "f21", IsHardFloatProcess())
+ .Cases("f22", "f23", "f24", "f25", "f26", "f27",
IsHardFloatProcess())
+ .Default(false);
----------------
What about if I have the D extension but only use LP64F as the ABI? Does it
matter that this says f9 is callee-saved but in reality only the low 32 bits
are?
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:16
+class ABISysV_riscv : public lldb_private::MCBasedABI {
+ bool isRV64;
+
----------------
IsRV64?
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:70-71
+ // Ensure addresses are smaller than XLEN bits wide. Calls can use the
least
+ // significant bit to store auxiliary information, so no strict check is
+ // done for alignment.
+ if (!isRV64)
----------------
2 and 3 mod 4 are nevertheless invalid if you don't have the C extension and
will take an alignment fault
================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:72-73
+ // done for alignment.
+ if (!isRV64)
+ return (pc <= UINT32_MAX);
+ return (pc <= UINT64_MAX);
----------------
Are addresses reliably zero-extended or can you ever get cases where they're
sign-extended from a valid 32-bit address to a 64-bit address?
================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1152
+ triple.getArch() == llvm::Triple::riscv64)
+ features_str += "+a,+c,+d,+f,+m";
+
----------------
This will override whatever the ELF says in its attributes section. This might
make sense as a default for if you don't have a binary and are just poking at
memory, but it makes things worse when you do, the common case that need to
work as best as we can manage.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62732/new/
https://reviews.llvm.org/D62732
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
