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

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D159101#4627705 <https://reviews.llvm.org/D159101#4627705>, @DavidSpickett wrote:

> Great. Could you include that in the commit message? Seems like we get a lot of questions about how mature risc-v support is, so we can point to this as a milestone for that.

Will do.



================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:107
+static size_t word_size = 4U;
+static size_t reg_size = word_size;
+
----------------
jasonmolenda wrote:
> ted wrote:
> > 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.
> I'd prefer that the CreateInstance method initialize an ivar with the word size based on the passed-in ArchSpec, or save the ArchSpec in an ivar.  risc-v seems to have many variants so I worry about saving the full ArchSpec when ABI::CreateInstance is being called, possibly where we might not have the most specific ArchSpec for this process?  But 32-bit v. 64-bit will surely be constant.  When I was reading the patch later I'd see references to `reg_size` and scratch my head where that variable was coming from.
> 
> This static will not work correctly if you have an lldb connected to an rv32 target and an rv64 target simultaneously.
> 
> Didn't the earlier ABI have a `isRV64` ivar (should have been m_is_rv64 but whatever) to solve this?
This is a very good point - debugging both rv32 and rv64 programs at the same time could give incorrect results. I think setting an ivar with 32-bit vs 64-bit in CreateInstance, and setting local variables based on that is a good idea.


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:638
+bool ABISysV_riscv::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
+  return false;
+}
----------------
jasonmolenda wrote:
> We were emailing a couple of months ago and I think I suggested something like this, based on the previous ABI patch
> 
> ```
> bool ABISysV_riscv::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
>   unwind_plan.Clear();
>   unwind_plan.SetRegisterKind(eRegisterKindGeneric);
> 
>   uint32_t pc_reg_num = LLDB_REGNUM_GENERIC_PC;
>   uint32_t fp_reg_num = LLDB_REGNUM_GENERIC_FP;
> 
>   UnwindPlan::RowSP row(new UnwindPlan::Row);
> 
>   // Define the CFA as the current frame pointer value.
>   row->GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 0);
>   row->SetOffset(0);
> 
>   int ptr_size = 4;
>   if (isRV64)
>     ptr_size = 8;
> 
>   // Assume the ra reg (return pc) and caller's frame pointer 
>   // have been spilled to stack already.
>   row->SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
>   row->SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);
> 
>   unwind_plan.AppendRow(row);
>   unwind_plan.SetSourceName("riscv default unwind plan");
>   unwind_plan.SetSourcedFromCompiler(eLazyBoolNo);
>   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
>   return true;
> }
> ```
The RISC-V ABI says the frame pointer is optional, and I haven't found a way to tell if the FP (register s0) is being used as an FP, or if it's a generic callee-saved register. 

Debugging an rv32 program that doesn't use the FP works with the current CreateDefaultUnwindPlan. I can step in, step over, step out and get a backtrace. Is there anything else I need to test?


================
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;
----------------
jasonmolenda wrote:
> DavidSpickett wrote:
> > Combine this into one if.
> Maybe more clear like
> ```
> if (arch_flags | ArchSpec::eRISCV_rvc && pc & 2) 
>   return false;
> ```
> 
> It's too bad ArchSpec doesn't have a `Flags()` method which returns a Flags object, so this could be  `if (arch.Flags().Test(ArchSpec::eRISCV_rvc) && pc & 2)`.
> 
> Suppose it would be just as easy to do 
> ```
>   Flags arch_flags(arch.GetFlags();
>   if (arch_flags.Test(ArchSpec::eRISCV_rvc) && pc & 2)
>     return false;
> ```
I like this idea.

Adding a Flags() method to ArchSpec is simple:

#include "lldb/Utility/Flags.h"
…
  Flags Flags() const { return lldb_private::Flags(m_flags); }

Do you think I should add that to this diff? Or I could go with your alternative:

Flags arch_flags(arch.GetFlags();
if (arch_flags.Test(ArchSpec::eRISCV_rvc) && pc & 2)
  return false;


What do you think?



================
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,";
   }
----------------
kito-cheng wrote:
> DavidSpickett wrote:
> > 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.
> RISC-V have ELF attribute* to record which arch used for this binary, so you may need to add something at `lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp` to read that and record something in ArchSpec like what `ObjectFileELF::ParseARMAttributes` do.
> 
> * https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_arch-5-ntbssubarch
The ArchSpec flags come from the ELF attributes. Unfortunately they don't have everything we need, specifically attributes for the "A" (atomic instructions) and "M" (integer multiplication and division) extensions, but I think pretty much everyone will have those, so having them always on is OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101