[lldb][RISCV] Fix return value reading

[sga-sc]

This patch fixes return value reading after thread step-out command on RISC-V architecture. Also it fixes TestReturnValue.py test

[llvmbot]

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-backend-risc-v

Author: Georgiy Samoylov (sga-sc)

Changes

This patch fixes return value reading after thread step-out command on RISC-V architecture. Also it fixes TestReturnValue.py test

---

Full diff: https://github.com/llvm/llvm-project/pull/163931.diff

2 Files Affected:

• (modified) lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp (+129-72)
• (modified) lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h (-4)

diff --git a/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp b/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
index 822c93dbbec3d..d0a1601e80469 100644
--- a/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
+++ b/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
@@ -136,6 +136,10 @@ const RegisterInfo *ABISysV_riscv::GetRegisterInfoArray(uint32_t &count) {
 // Static Functions
 //------------------------------------------------------------------
 
+static inline size_t DeriveRegSizeInBytes(bool is_rv64) {
+  return is_rv64 ? 8 : 4;
+}
+
 ABISP
 ABISysV_riscv::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
   llvm::Triple::ArchType machine = arch.GetTriple().getArch();
@@ -151,14 +155,14 @@ ABISysV_riscv::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
 }
 
 static inline size_t AugmentArgSize(bool is_rv64, size_t size_in_bytes) {
-  size_t word_size = is_rv64 ? 8 : 4;
+  size_t word_size = DeriveRegSizeInBytes(is_rv64);
   return llvm::alignTo(size_in_bytes, word_size);
 }
 
 static size_t
 TotalArgsSizeInWords(bool is_rv64,
                      const llvm::ArrayRef<ABI::CallArgument> &args) {
-  size_t reg_size = is_rv64 ? 8 : 4;
+  size_t reg_size = DeriveRegSizeInBytes(is_rv64);
   size_t word_size = reg_size;
   size_t total_size = 0;
   for (const auto &arg : args)
@@ -275,7 +279,7 @@ bool ABISysV_riscv::PrepareTrivialCall(
   if (!process)
     return false;
 
-  size_t reg_size = m_is_rv64 ? 8 : 4;
+  size_t reg_size = DeriveRegSizeInBytes(m_is_rv64);
   size_t word_size = reg_size;
   // Push host data onto target.
   for (const auto &arg : args) {
@@ -397,7 +401,7 @@ Status ABISysV_riscv::SetReturnValueObject(StackFrameSP &frame_sp,
     return result;
   }
 
-  size_t reg_size = m_is_rv64 ? 8 : 4;
+  size_t reg_size = DeriveRegSizeInBytes(m_is_rv64);
   if (num_bytes <= 2 * reg_size) {
     offset_t offset = 0;
     uint64_t raw_value = data.GetMaxU64(&offset, num_bytes);
@@ -490,9 +494,11 @@ static bool SetSizedFloat(Scalar &scalar, uint64_t raw_value,
 static ValueObjectSP GetValObjFromIntRegs(Thread &thread,
                                           const RegisterContextSP &reg_ctx,
                                           llvm::Triple::ArchType machine,
-                                          uint32_t type_flags,
+                                          const CompilerType &compiler_type,
                                           uint32_t byte_size) {
   Value value;
+  value.SetCompilerType(compiler_type);
+
   ValueObjectSP return_valobj_sp;
   auto reg_info_a0 =
       reg_ctx->GetRegisterInfo(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_ARG1);
@@ -545,11 +551,11 @@ static ValueObjectSP GetValObjFromIntRegs(Thread &thread,
     return return_valobj_sp;
   }
 
-  if (type_flags & eTypeIsInteger) {
-    const bool is_signed = (type_flags & eTypeIsSigned) != 0;
+  if (compiler_type.IsInteger()) {
+    const bool is_signed = compiler_type.IsSigned();
     if (!SetSizedInteger(value.GetScalar(), raw_value, byte_size, is_signed))
       return return_valobj_sp;
-  } else if (type_flags & eTypeIsFloat) {
+  } else if (compiler_type.IsFloat()) {
     if (!SetSizedFloat(value.GetScalar(), raw_value, byte_size))
       return return_valobj_sp;
   } else
@@ -564,7 +570,7 @@ static ValueObjectSP GetValObjFromIntRegs(Thread &thread,
 static ValueObjectSP
 GetValObjFromFPRegs(Thread &thread, const RegisterContextSP &reg_ctx,
                     llvm::Triple::ArchType machine, uint32_t arch_fp_flags,
-                    uint32_t type_flags, uint32_t byte_size) {
+                    CompilerType &compiler_type, uint32_t byte_size) {
   auto reg_info_fa0 = reg_ctx->GetRegisterInfoByName("fa0");
   bool use_fp_regs = false;
   ValueObjectSP return_valobj_sp;
@@ -572,8 +578,8 @@ GetValObjFromFPRegs(Thread &thread, const RegisterContextSP &reg_ctx,
   switch (arch_fp_flags) {
   // fp return value in integer registers a0 and possibly a1
   case ArchSpec::eRISCV_float_abi_soft:
-    return_valobj_sp =
-        GetValObjFromIntRegs(thread, reg_ctx, machine, type_flags, byte_size);
+    return_valobj_sp = GetValObjFromIntRegs(thread, reg_ctx, machine,
+                                            compiler_type, byte_size);
     return return_valobj_sp;
   // fp return value in fp register fa0 (only float)
   case ArchSpec::eRISCV_float_abi_single:
@@ -602,7 +608,104 @@ GetValObjFromFPRegs(Thread &thread, const RegisterContextSP &reg_ctx,
                                           value, ConstString(""));
   }
   // we should never reach this, but if we do, use the integer registers
-  return GetValObjFromIntRegs(thread, reg_ctx, machine, type_flags, byte_size);
+  return GetValObjFromIntRegs(thread, reg_ctx, machine, compiler_type,
+                              byte_size);
+}
+
+static DataExtractor CopyReturnValueToBuffer(ExecutionContext &exe_ctx,
+                                             RegisterValue &a0_reg_value,
+                                             RegisterValue &a1_reg_value,
+                                             const size_t xlen_byte_size,
+                                             const uint32_t byte_size) {
+
+  DataExtractor a0_extractor;
+  DataExtractor a1_extractor;
+
+  // RISC-V ABI stands:
+  // "Aggregates whose total size is no more than XLEN bits
+  // are passed in a register, with the fields laid out as though
+  // they were passed in memory."
+  if (byte_size <= xlen_byte_size) {
+    // value is stored only in a0
+    a0_reg_value.GetData(a0_extractor);
+    // shrink data to byte_size length
+    DataExtractor data{a0_extractor, 0, byte_size};
+    return data;
+  }
+
+  // "Aggregates whose total size is no more than 2×XLEN bits
+  // are passed in a pair of registers;"
+  if (byte_size <= 2 * xlen_byte_size) {
+    // value is stored in a0 and a1 consequently
+    a0_reg_value.GetData(a0_extractor);
+    a1_reg_value.GetData(a1_extractor);
+    a0_extractor.Append(a1_extractor);
+    // shrink data to byte_size length
+    DataExtractor data{a0_extractor, 0, byte_size};
+    return data;
+  }
+
+  // "Aggregates larger than 2×XLEN bits are passed by reference
+  // and are replaced in the argument list with the address"
+  const lldb::addr_t value_addr =
+      a1_reg_value.GetAsUInt64(LLDB_INVALID_ADDRESS, nullptr);
+
+  if (value_addr == LLDB_INVALID_ADDRESS)
+    return DataExtractor{};
+
+  Status error;
+  WritableDataBufferSP data_sp(new DataBufferHeap(byte_size, 0));
+  if (exe_ctx.GetProcessRef().ReadMemory(value_addr, data_sp->GetBytes(),
+                                         byte_size, error) != byte_size ||
+      error.Fail())
+    return DataExtractor{};
+
+  DataExtractor data;
+  data.SetData(data_sp);
+  return data;
+}
+
+static ValueObjectSP GetAggregateObj(Thread &thread, RegisterContextSP reg_ctx,
+                                     const CompilerType &compiler_type,
+                                     const size_t xlen_byte_size,
+                                     const uint32_t byte_size) {
+  const RegisterInfo *a0_reg_info =
+      reg_ctx->GetRegisterInfo(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_ARG1);
+
+  const RegisterInfo *a1_reg_info =
+      reg_ctx->GetRegisterInfo(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_ARG2);
+
+  if (!a0_reg_info || !a1_reg_info)
+    return ValueObjectSP{};
+
+  RegisterValue a0_reg_value;
+  RegisterValue a1_reg_value;
+
+  if (!reg_ctx->ReadRegister(a0_reg_info, a0_reg_value) ||
+      !reg_ctx->ReadRegister(a1_reg_info, a1_reg_value))
+    return ValueObjectSP{};
+
+  if (a0_reg_value.GetType() == RegisterValue::Type::eTypeInvalid ||
+      a1_reg_value.GetType() == RegisterValue::Type::eTypeInvalid)
+    return ValueObjectSP{};
+
+  ExecutionContext exe_ctx(thread.shared_from_this());
+  DataExtractor data = CopyReturnValueToBuffer(
+      exe_ctx, a0_reg_value, a1_reg_value, xlen_byte_size, byte_size);
+
+  if (data.GetByteSize() != byte_size)
+    return ValueObjectSP{};
+
+  const ByteOrder byte_order = exe_ctx.GetProcessRef().GetByteOrder();
+  const uint32_t address_byte_size =
+      exe_ctx.GetProcessRef().GetAddressByteSize();
+  data.SetByteOrder(byte_order);
+  data.SetAddressByteSize(address_byte_size);
+
+  ValueObjectSP return_valobj_sp =
+      ValueObjectConstResult::Create(thread.GetStackFrameAtIndex(0).get(),
+                                     compiler_type, ConstString(""), data);
+  return return_valobj_sp;
 }
 
 ValueObjectSP
@@ -617,23 +720,21 @@ ABISysV_riscv::GetReturnValueObjectSimple(Thread &thread,
   if (!reg_ctx)
     return return_valobj_sp;
 
-  Value value;
-  value.SetCompilerType(compiler_type);
-
-  const uint32_t type_flags = compiler_type.GetTypeInfo();
   const size_t byte_size =
       llvm::expectedToOptional(compiler_type.GetByteSize(&thread)).value_or(0);
   const ArchSpec arch = thread.GetProcess()->GetTarget().GetArchitecture();
   const llvm::Triple::ArchType machine = arch.GetMachine();
 
   // Integer return type.
-  if (type_flags & eTypeIsInteger) {
-    return_valobj_sp =
-        GetValObjFromIntRegs(thread, reg_ctx, machine, type_flags, byte_size);
+  if (compiler_type.IsInteger()) {
+    return_valobj_sp = GetValObjFromIntRegs(thread, reg_ctx, machine,
+                                            compiler_type, byte_size);
     return return_valobj_sp;
   }
   // Pointer return type.
-  else if (type_flags & eTypeIsPointer) {
+  if (compiler_type.IsPointerType()) {
+    Value value;
+    value.SetCompilerType(compiler_type);
     auto reg_info_a0 = reg_ctx->GetRegisterInfo(eRegisterKindGeneric,
                                                 LLDB_REGNUM_GENERIC_ARG1);
     value.GetScalar() = reg_ctx->ReadRegisterAsUnsigned(reg_info_a0, 0);
@@ -642,7 +743,7 @@ ABISysV_riscv::GetReturnValueObjectSimple(Thread &thread,
                                           value, ConstString(""));
   }
   // Floating point return type.
-  else if (type_flags & eTypeIsFloat) {
+  if (compiler_type.IsFloat()) {
     uint32_t float_count = 0;
     bool is_complex = false;
 
@@ -651,58 +752,16 @@ ABISysV_riscv::GetReturnValueObjectSimple(Thread &thread,
       const uint32_t arch_fp_flags =
           arch.GetFlags() & ArchSpec::eRISCV_float_abi_mask;
       return_valobj_sp = GetValObjFromFPRegs(
-          thread, reg_ctx, machine, arch_fp_flags, type_flags, byte_size);
+          thread, reg_ctx, machine, arch_fp_flags, compiler_type, byte_size);
       return return_valobj_sp;
     }
   }
-  // Unsupported return type.
-  return return_valobj_sp;
-}
-
-ValueObjectSP
-ABISysV_riscv::GetReturnValueObjectImpl(lldb_private::Thread &thread,
-                                        llvm::Type &type) const {
-  Value value;
-  ValueObjectSP return_valobj_sp;
-
-  auto reg_ctx = thread.GetRegisterContext();
-  if (!reg_ctx)
-    return return_valobj_sp;
+  // Aggregate return type
+  if (compiler_type.IsAggregateType()) {
+    size_t xlen_byte_size = DeriveRegSizeInBytes(m_is_rv64);
 
-  uint32_t type_flags = 0;
-  if (type.isIntegerTy())
-    type_flags = eTypeIsInteger;
-  else if (type.isVoidTy())
-    type_flags = eTypeIsPointer;
-  else if (type.isFloatTy())
-    type_flags = eTypeIsFloat;
-
-  const uint32_t byte_size = type.getPrimitiveSizeInBits() / CHAR_BIT;
-  const ArchSpec arch = thread.GetProcess()->GetTarget().GetArchitecture();
-  const llvm::Triple::ArchType machine = arch.GetMachine();
-
-  // Integer return type.
-  if (type_flags & eTypeIsInteger) {
-    return_valobj_sp =
-        GetValObjFromIntRegs(thread, reg_ctx, machine, type_flags, byte_size);
-    return return_valobj_sp;
-  }
-  // Pointer return type.
-  else if (type_flags & eTypeIsPointer) {
-    auto reg_info_a0 = reg_ctx->GetRegisterInfo(eRegisterKindGeneric,
-                                                LLDB_REGNUM_GENERIC_ARG1);
-    value.GetScalar() = reg_ctx->ReadRegisterAsUnsigned(reg_info_a0, 0);
-    value.SetValueType(Value::ValueType::Scalar);
-    return ValueObjectConstResult::Create(thread.GetStackFrameAtIndex(0).get(),
-                                          value, ConstString(""));
-  }
-  // Floating point return type.
-  else if (type_flags & eTypeIsFloat) {
-    const uint32_t arch_fp_flags =
-        arch.GetFlags() & ArchSpec::eRISCV_float_abi_mask;
-    return_valobj_sp = GetValObjFromFPRegs(
-        thread, reg_ctx, machine, arch_fp_flags, type_flags, byte_size);
-    return return_valobj_sp;
+    return GetAggregateObj(thread, reg_ctx, compiler_type, xlen_byte_size,
+                           byte_size);
   }
   // Unsupported return type.
   return return_valobj_sp;
@@ -748,9 +807,7 @@ UnwindPlanSP ABISysV_riscv::CreateDefaultUnwindPlan() {
   // Define the CFA as the current frame pointer value.
   row.GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 0);
 
-  int reg_size = 4;
-  if (m_is_rv64)
-    reg_size = 8;
+  int reg_size = DeriveRegSizeInBytes(m_is_rv64);
 
   // Assume the ra reg (return pc) and caller's frame pointer 
   // have been spilled to stack already.
diff --git a/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h b/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
index 539a45de5ee77..39462aa851403 100644
--- a/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
+++ b/lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -47,10 +47,6 @@ class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
   GetReturnValueObjectImpl(lldb_private::Thread &thread,
                            lldb_private::CompilerType &type) const override;
 
-  // Specialized to work with llvm IR types.
-  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
-                                               llvm::Type &type) const override;
-
   lldb::UnwindPlanSP CreateFunctionEntryUnwindPlan() override;
 
   lldb::UnwindPlanSP CreateDefaultUnwindPlan() override;

[DavidSpickett]

This patch fixes return value reading after thread step-out command on RISC-V architecture. Also it fixes TestReturnValue.py test

Can you reword this to make it clear whether the changes for the first part also fix the test, or you added more changes to fix the test? For example:

"This patch fixes return value reading after thread step-out command on RISC-V architecture. In doing so, TestReturnValue.py which was previously failing, is now passing."

I've only glanced at TestReturnValue.py but it looks quite comprehensive. So if that is covering the different scenarios then that's fine. Just want to make sure that's the case.

The scenarios being < xlen, <=2*xlen, and aggregate I think.

[DavidSpickett]

Why a1 not a0?

[DavidSpickett]

If that's what the ABI does, also include quotes for that.

[sga-sc]

Thank you for pointing out to this code! I will add an explanation of problems that I encountered with during testing this.

[DavidSpickett]

Also thank you for doing this as 2 commits. I read the combined changes at first and thought about asking you to split it but that would have been a real pain for you to do, then I saw you'd already done it!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[sga-sc]

Consider compiling this code for RISC-V 64 bit:

struct five_int
{
  int one_field;
  int two_field;
  int three_field;
  int four_field;
  int five_field;
};

struct five_int
return_five_int (struct five_int value)
{
  return value;
}

int main()
{
	struct five_int arg = {10, 20, 30, 40, 50};
	return_five_int(arg);
	return 0;
}

The byte size of struct five_int's is greater than 2*xlen (20 > 16). According to the RISC-V calling convention, in this case return value is passed by reference. Caller (main) allocates memory on the stack and passes the address as an implicit first parameter. Consider that we are LLDB and we try to find out what the callee (return_five_int) returned after stepping out from it. The return value address was stored in a0 at the start of callee function. However after the callee's prologue, we can't assume that a0 holds the same value. There is also a note in RISC-V ABI about it.

@lenary @DavidSpickett What should we do in this case? How can we get an access to the address of return value?

At this moment I read a1 value and the test passes just by accident.

[DavidSpickett]

I tried your example on AArch64 and we don't get a return value there either. TestReturnValues.py in fact skips checking a type called return_five_int for the SysV AArch64 ABI (and now I see where you got the example from).

According to the AArch64 AAPCS (it's ABI):

B.4 If the argument type is a Composite Type that is larger than 16 bytes, then the argument is copied to memory allocated by the caller and the argument is replaced by a pointer to the copy.

And then https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#69result-return which says:

Otherwise, the caller shall reserve a block of memory of sufficient size and alignment to hold the result. The address of the memory block shall be passed as an additional argument to the function in x8. The callee may modify the result memory block at any point during the execution of the subroutine (there is no requirement for the callee to preserve the value stored in x8).

(emphasis mine)

This matches the statement in the RISC-V ABI, so I would be fine with you adding RISC-V to the skip list there.

[topperc]

What about aggregates of float or double under the floating point ABIs? Those would be passed in float registers.