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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda 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;
+
----------------
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?


================
Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:638
+bool ABISysV_riscv::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
+  return false;
+}
----------------
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;
}
```


================
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;
----------------
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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101