[lldb][PPC64] Fixed vector and struct return value

[luporl]

Summary

The PowerPC64 ABI plugin was modified to:

• properly handle vector type return values
• implement support for struct/class return values

A refactoring in the code that handles return values was also performed, to make it possible to handle structs without repeating (when possible) code that handles its fields.

There was also an issue with CreateInstance(), that only created an instance in the first time it was called and then cached it in a static var. When restarting a process under LLDB's control, the ABI's process weak pointer would become null, and using it would result in a segmentation fault. This issue became more evident after the latest changes to PPC64 plugin, that now uses the process pointer to get the target byte order, making LLDB to seg fault when restarting a program. This was fixed by making CreateInstance() to always create a new ABI instance.

All of LLDB's ReturnValue tests are passing for PPC64le now. It should work for PPC64be too, although this was not tested.

[labath]

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

393–421 ↗

(On Diff #131222)

The LLDB_LOG macro might be of interest to you. It will not be *as* concise as your solution here, but it should get you most of the benefits without having to reinvent your own logging infrastructure.

551–579 ↗

(On Diff #131222)

Instead of the constructor+init combo you can use a static factory function which returns llvm::Expected<ReturnValueExtractor>. The factory function can do all the operations that can fail, and then construct the object using a simple (possibly private) constructor. That way you never end up having a partially initialized object.

709 ↗

(On Diff #131222)

Should this really be GetDouble?

[luporl]

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

393–421 ↗

(On Diff #131222)

Ok, thanks, I'll take a look at it.

551–579 ↗

(On Diff #131222)

Ok, I'll make this change.

709 ↗

(On Diff #131222)

Yes, it must really be GetDouble.
It seems that PowerPC64 always stores floating point values as doubles in its FP registers, even float ones.
The previous code, that used GetFloat, was not working properly.

[labath]

Also please make sure to run clang format over this change.

[luporl]

• Addressed review comments.

Note: I ran clang-format over the whole file. While most of the formatting was
done over the previous diff, it also caught a few lines that were not part
of it. I guess this should be ok?

[clayborg]

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

449 ↗

(On Diff #131274)

Seems like this function is so similar to GetFPRName, both contents can be inlined into GetName() and the code can be:

std::string regName;
llvm::raw_string_ostream ss(regName);
ss << m_type == GRP ? "r" : "f" << m_index + 3;
ss.flush();

457 ↗

(On Diff #131274)

Seems like this function is so similar to GetGPRName, both contents can be inlined into GetName() as noted above?

588 ↗

(On Diff #131274)

"ProcessSP process_sp" argument isn't needed as you can call "thread.GetProcess()" to get it.

[luporl]

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

449 ↗

(On Diff #131274)

Right, a single GetName() function is probably enough. There is only another minor difference that must be taken into account, that is the initial index offset, that begins at 3 for GPRs and at 1 for FPRs.
So the resulting code could be:

std::string regName;
llvm::raw_string_ostream ss(regName);
if (m_type == GPR)
  ss << "r" << m_index + 3;
else
  ss << "f" << m_index + 1;
ss.flush();

588 ↗

(On Diff #131274)

Well, I'm using "thread.GetProcess()" in the Create() factory method instead, because it seems better to return an error if I get a null pointer from GetProcess().
And if I move this to the constructor and call thread.GetProcess().GetByteOrder() to initialize m_byte_order, then there is the risk of dereferencing a null pointer.

Unless this can never happen when the ABI plugin is invoked to get a return value object, then there would be no problem.
Is this the case?

[labath]

Normally you should only format the lines you touch (if you use git, there's git clang-format command for that), but as you're pretty much rewriting this file anyway, it probably does not matter in this case.

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

951–956 ↗

(On Diff #131274)

As of now, we have an LLDB_LOG_ERROR macro, so you can write this as:

LLDB_LOG_ERROR(log, exp_extractor.takeError(), "Extracting return value failed: {0}");

[luporl]

Now using LLDB_LOG_ERROR.

There was also an issue in HostInfoBase, where there was a missing
ppc64le case in a switch, which would end up making lldb-server abort
because it wouldn't find a valid arch.

Everything was working before probably because the old arch detection
code was thinking the arch was ppc64 at that point, but as it changed
(probably related to this: D42488) and now correctly returns ppc64le, the issue was found.

[labath]

I think this looks good. Thank you for standing by.

source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

454 ↗

(On Diff #132211)

This can be even shorter, like: return ("r" + llvm::Twine(m_index+3)).str(); The str() may not even be necessary (I don't know if Twine is implicitly convertible here off-hand).

[luporl]

Nice, thanks for the suggestion @labath!
This should reduce 5 lines of code to one.
I will also combine the two GetRegName() methods into one, as Greg suggested, that I haven't done yet.

I shall send the new diff soon.

[luporl]

• Made the get register name code shorter.
• Merge branch 'master' into fix-return-value

[luporl]

Is there anything pending on this?

[labath]

It doesn't look like there is. Do you need someone to check this in?

[luporl]

Yes, could you check this in for me? Thanks!

lldb/trunk/source/Host/common/HostInfoBase.cpp

void HostInfoBase::ComputeHostArchitectureSupport(ArchSpec &arch_32,
 
   switch (triple.getArch()) {
   default:
     arch_32.SetTriple(triple);
     break;
 
   case llvm::Triple::aarch64:
   case llvm::Triple::ppc64:
+  case llvm::Triple::ppc64le:
   case llvm::Triple::x86_64:
     arch_64.SetTriple(triple);
     arch_32.SetTriple(triple.get32BitArchVariant());
     break;
 
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
   case llvm::Triple::sparcv9:
   case llvm::Triple::systemz:
     arch_64.SetTriple(triple);
     break;
   }
 }

lldb/trunk/source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp

 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Triple.h"
 
 // Project includes
+#include "Utility/PPC64LE_DWARF_Registers.h"
+#include "Utility/PPC64_DWARF_Registers.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Core/ValueObjectMemory.h"
 #include "lldb/Core/ValueObjectRegister.h"
+#include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Status.h"
-#include "Utility/PPC64_DWARF_Registers.h"
-#include "Utility/PPC64LE_DWARF_Registers.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/Decl.h"
 
 #define DECLARE_REGISTER_INFOS_PPC64_STRUCT
 #include "Plugins/Process/Utility/RegisterInfos_ppc64.h"
 #undef DECLARE_REGISTER_INFOS_PPC64_STRUCT
 
 #define DECLARE_REGISTER_INFOS_PPC64LE_STRUCT
 #include "Plugins/Process/Utility/RegisterInfos_ppc64le.h"
 #undef DECLARE_REGISTER_INFOS_PPC64LE_STRUCT
 
 using namespace lldb;
 using namespace lldb_private;
 
 const lldb_private::RegisterInfo *
 ABISysV_ppc64::GetRegisterInfoArray(uint32_t &count) {
-  if (GetByteOrder() == lldb::eByteOrderLittle)
-  {
-    count = llvm::array_lengthof (g_register_infos_ppc64le);
+  if (GetByteOrder() == lldb::eByteOrderLittle) {
+    count = llvm::array_lengthof(g_register_infos_ppc64le);
     return g_register_infos_ppc64le;
   } else {
-    count = llvm::array_lengthof (g_register_infos_ppc64);
+    count = llvm::array_lengthof(g_register_infos_ppc64);
     return g_register_infos_ppc64;
   }
 }
 
 size_t ABISysV_ppc64::GetRedZoneSize() const { return 224; }
 
 lldb::ByteOrder ABISysV_ppc64::GetByteOrder() const {
   return GetProcessSP()->GetByteOrder();
 }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
-ABISysV_ppc64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
+ABISysV_ppc64::CreateInstance(lldb::ProcessSP process_sp,
+                              const ArchSpec &arch) {
   if (arch.GetTriple().getArch() == llvm::Triple::ppc64 ||
       arch.GetTriple().getArch() == llvm::Triple::ppc64le) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_ppc64(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_ppc64(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_ppc64::PrepareTrivialCall(Thread &thread, addr_t sp,
                                        addr_t func_addr, addr_t return_addr,
                                        llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
bool ABISysV_ppc64::PrepareTrivialCall(Thread &thread, addr_t sp,
   const RegisterInfo *lr_reg_info =
       reg_ctx->GetRegisterInfo(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_RA);
   const RegisterInfo *r2_reg_info = reg_ctx->GetRegisterInfoAtIndex(2);
   const RegisterInfo *r12_reg_info = reg_ctx->GetRegisterInfoAtIndex(12);
 
   // Save return address onto the stack.
   if (log)
     log->Printf("Pushing the return address onto the stack: 0x%" PRIx64
-                "(+16): 0x%" PRIx64, (uint64_t)sp, (uint64_t)return_addr);
+                "(+16): 0x%" PRIx64,
+                (uint64_t)sp, (uint64_t)return_addr);
   if (!process_sp->WritePointerToMemory(sp + 16, return_addr, error))
     return false;
 
   // Write the return address to link register.
   if (log)
     log->Printf("Writing LR: 0x%" PRIx64, (uint64_t)return_addr);
   if (!reg_ctx->WriteRegisterFromUnsigned(lr_reg_info, return_addr))
     return false;
bool ABISysV_ppc64::PrepareTrivialCall(Thread &thread, addr_t sp,
   uint64_t stack_offset;
   if (GetByteOrder() == lldb::eByteOrderLittle)
     stack_offset = 24;
   else
     stack_offset = 40;
 
   if (log)
     log->Printf("Writing R2 (TOC) at SP(0x%" PRIx64 ")+%d: 0x%" PRIx64,
-        (uint64_t)(sp + stack_offset), (int) stack_offset, (uint64_t)reg_value);
+                (uint64_t)(sp + stack_offset), (int)stack_offset,
+                (uint64_t)reg_value);
   if (!process_sp->WritePointerToMemory(sp + stack_offset, reg_value, error))
     return false;
 
   // Read the current SP value.
   reg_value = reg_ctx->ReadRegisterAsUnsigned(sp_reg_info, 0);
 
   // Save current SP onto the stack.
   if (log)
-    log->Printf("Writing SP at SP(0x%" PRIx64 ")+0: 0x%" PRIx64,
-      (uint64_t)sp, (uint64_t)reg_value);
+    log->Printf("Writing SP at SP(0x%" PRIx64 ")+0: 0x%" PRIx64, (uint64_t)sp,
+                (uint64_t)reg_value);
   if (!process_sp->WritePointerToMemory(sp, reg_value, error))
     return false;
 
   // %r1 is set to the actual stack value.
   if (log)
     log->Printf("Writing SP: 0x%" PRIx64, (uint64_t)sp);
 
   if (!reg_ctx->WriteRegisterFromUnsigned(sp_reg_info, sp))
bool ABISysV_ppc64::GetArgumentValues(Thread &thread, ValueList &values) const {
   else
     stack_offset = 48;
 
   // jump over return address.
   addr_t current_stack_argument = sp + stack_offset;
   uint32_t argument_register_ids[8];
 
   for (size_t i = 0; i < 8; ++i) {
-    argument_register_ids[i] = reg_ctx->GetRegisterInfo(eRegisterKindGeneric,
-        LLDB_REGNUM_GENERIC_ARG1 + i)->kinds[eRegisterKindLLDB];
+    argument_register_ids[i] =
+        reg_ctx
+            ->GetRegisterInfo(eRegisterKindGeneric,
+                              LLDB_REGNUM_GENERIC_ARG1 + i)
+            ->kinds[eRegisterKindLLDB];
   }
 
   unsigned int current_argument_register = 0;
 
   for (value_index = 0; value_index < num_values; ++value_index) {
     Value *value = values.GetValueAtIndex(value_index);
 
     if (!value)
if (!set_it_simple) {
     // We should figure out where it really goes, but we don't support this yet.
     error.SetErrorString("We only support setting simple integer and float "
                          "return types at present.");
   }
 
   return error;
 }
 
-ValueObjectSP ABISysV_ppc64::GetReturnValueObjectSimple(
-    Thread &thread, CompilerType &return_compiler_type) const {
-  ValueObjectSP return_valobj_sp;
-  Value value;
+//
+// ReturnValueExtractor
+//
+
+namespace {
+
+#define LOG_PREFIX "ReturnValueExtractor: "
 
-  if (!return_compiler_type)
-    return return_valobj_sp;
+class ReturnValueExtractor {
+  // This class represents a register, from which data may be extracted.
+  //
+  // It may be constructed by directly specifying its index (where 0 is
+  // the first register used to return values) or by specifying the offset
+  // of a given struct field, in which case the appropriated register index
+  // will be calculated.
+  class Register {
+  public:
+    enum Type {
+      GPR, // General Purpose Register
+      FPR  // Floating Point Register
+    };
 
-  // value.SetContext (Value::eContextTypeClangType, return_value_type);
-  value.SetCompilerType(return_compiler_type);
+    // main constructor
+    //
+    // offs - field offset in struct
+    Register(Type ty, uint32_t index, uint32_t offs, RegisterContext *reg_ctx,
+             ByteOrder byte_order)
+        : m_index(index), m_offs(offs % sizeof(uint64_t)),
+          m_avail(sizeof(uint64_t) - m_offs), m_type(ty), m_reg_ctx(reg_ctx),
+          m_byte_order(byte_order) {}
+
+    // explicit index, no offset
+    Register(Type ty, uint32_t index, RegisterContext *reg_ctx,
+             ByteOrder byte_order)
+        : Register(ty, index, 0, reg_ctx, byte_order) {}
+
+    // GPR, calculate index from offs
+    Register(uint32_t offs, RegisterContext *reg_ctx, ByteOrder byte_order)
+        : Register(GPR, offs / sizeof(uint64_t), offs, reg_ctx, byte_order) {}
+
+    uint32_t Index() const { return m_index; }
+
+    // register offset where data is located
+    uint32_t Offs() const { return m_offs; }
+
+    // available bytes in this register
+    uint32_t Avail() const { return m_avail; }
+
+    bool IsValid() const {
+      if (m_index > 7) {
+        LLDB_LOG(m_log, LOG_PREFIX
+                 "No more than 8 registers should be used to return values");
+        return false;
+      }
+      return true;
+    }
+
+    std::string GetName() const {
+      if (m_type == GPR)
+        return ("r" + llvm::Twine(m_index + 3)).str();
+      else
+        return ("f" + llvm::Twine(m_index + 1)).str();
+    }
+
+    // get raw register data
+    bool GetRawData(uint64_t &raw_data) {
+      const RegisterInfo *reg_info =
+          m_reg_ctx->GetRegisterInfoByName(GetName());
+      if (!reg_info) {
+        LLDB_LOG(m_log, LOG_PREFIX "Failed to get RegisterInfo");
+        return false;
+      }
+
+      RegisterValue reg_val;
+      if (!m_reg_ctx->ReadRegister(reg_info, reg_val)) {
+        LLDB_LOG(m_log, LOG_PREFIX "ReadRegister() failed");
+        return false;
+      }
+
+      Status error;
+      uint32_t rc = reg_val.GetAsMemoryData(
+          reg_info, &raw_data, sizeof(raw_data), m_byte_order, error);
+      if (rc != sizeof(raw_data)) {
+        LLDB_LOG(m_log, LOG_PREFIX "GetAsMemoryData() failed");
+        return false;
+      }
 
+      return true;
+    }
+
+  private:
+    uint32_t m_index;
+    uint32_t m_offs;
+    uint32_t m_avail;
+    Type m_type;
+    RegisterContext *m_reg_ctx;
+    ByteOrder m_byte_order;
+    Log *m_log =
+        lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+  };
+
+  Register GetGPR(uint32_t index) const {
+    return Register(Register::GPR, index, m_reg_ctx, m_byte_order);
+  }
+
+  Register GetFPR(uint32_t index) const {
+    return Register(Register::FPR, index, m_reg_ctx, m_byte_order);
+  }
+
+  Register GetGPRByOffs(uint32_t offs) const {
+    return Register(offs, m_reg_ctx, m_byte_order);
+  }
+
+public:
+  // factory
+  static llvm::Expected<ReturnValueExtractor> Create(Thread &thread,
+                                                     CompilerType &type) {
     RegisterContext *reg_ctx = thread.GetRegisterContext().get();
     if (!reg_ctx)
-    return return_valobj_sp;
+      return llvm::make_error<llvm::StringError>(
+          LOG_PREFIX "Failed to get RegisterContext",
+          llvm::inconvertibleErrorCode());
 
-  const uint32_t type_flags = return_compiler_type.GetTypeInfo();
-  if (type_flags & eTypeIsScalar) {
-    value.SetValueType(Value::eValueTypeScalar);
+    ProcessSP process_sp = thread.GetProcess();
+    if (!process_sp)
+      return llvm::make_error<llvm::StringError>(
+          LOG_PREFIX "GetProcess() failed", llvm::inconvertibleErrorCode());
 
-    bool success = false;
+    return ReturnValueExtractor(thread, type, reg_ctx, process_sp);
+  }
+
+  // main method: get value of the type specified at construction time
+  ValueObjectSP GetValue() {
+    const uint32_t type_flags = m_type.GetTypeInfo();
+
+    // call the appropriate type handler
+    ValueSP value_sp;
+    ValueObjectSP valobj_sp;
+    if (type_flags & eTypeIsScalar) {
       if (type_flags & eTypeIsInteger) {
-      // Extract the register context so we can read arguments from registers
+        value_sp = GetIntegerValue(0);
+      } else if (type_flags & eTypeIsFloat) {
+        if (type_flags & eTypeIsComplex) {
+          LLDB_LOG(m_log, LOG_PREFIX "Complex numbers are not supported yet");
+          return ValueObjectSP();
+        } else {
+          value_sp = GetFloatValue(m_type, 0);
+        }
+      }
+    } else if (type_flags & eTypeIsPointer) {
+      value_sp = GetPointerValue(0);
+    }
 
-      const size_t byte_size = return_compiler_type.GetByteSize(nullptr);
-      uint64_t raw_value = thread.GetRegisterContext()->ReadRegisterAsUnsigned(
-          reg_ctx->GetRegisterInfoByName("r3", 0), 0);
-      const bool is_signed = (type_flags & eTypeIsSigned) != 0;
-      switch (byte_size) {
-      default:
-        break;
+    if (value_sp) {
+      valobj_sp = ValueObjectConstResult::Create(
+          m_thread.GetStackFrameAtIndex(0).get(), *value_sp, ConstString(""));
+    } else if (type_flags & eTypeIsVector) {
+      valobj_sp = GetVectorValueObject();
+    } else if (type_flags & eTypeIsStructUnion || type_flags & eTypeIsClass) {
+      valobj_sp = GetStructValueObject();
+    }
+
+    return valobj_sp;
+  }
+
+private:
+  // data
+  Thread &m_thread;
+  CompilerType &m_type;
+  uint64_t m_byte_size;
+  std::unique_ptr<DataBufferHeap> m_data_ap;
+  int32_t m_src_offs = 0;
+  int32_t m_dst_offs = 0;
+  bool m_packed = false;
+  Log *m_log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+  RegisterContext *m_reg_ctx;
+  ProcessSP m_process_sp;
+  ByteOrder m_byte_order;
+  uint32_t m_addr_size;
+
+  // methods
+
+  // constructor
+  ReturnValueExtractor(Thread &thread, CompilerType &type,
+                       RegisterContext *reg_ctx, ProcessSP process_sp)
+      : m_thread(thread), m_type(type),
+        m_byte_size(m_type.GetByteSize(nullptr)),
+        m_data_ap(new DataBufferHeap(m_byte_size, 0)), m_reg_ctx(reg_ctx),
+        m_process_sp(process_sp), m_byte_order(process_sp->GetByteOrder()),
+        m_addr_size(
+            process_sp->GetTarget().GetArchitecture().GetAddressByteSize()) {}
+
+  // build a new scalar value
+  ValueSP NewScalarValue(CompilerType &type) {
+    ValueSP value_sp(new Value);
+    value_sp->SetCompilerType(type);
+    value_sp->SetValueType(Value::eValueTypeScalar);
+    return value_sp;
+  }
+
+  // get an integer value in the specified register
+  ValueSP GetIntegerValue(uint32_t reg_index) {
+    uint64_t raw_value;
+    auto reg = GetGPR(reg_index);
+    if (!reg.GetRawData(raw_value))
+      return ValueSP();
 
-      case sizeof(uint64_t):
-        if (is_signed)
-          value.GetScalar() = (int64_t)(raw_value);
-        else
-          value.GetScalar() = (uint64_t)(raw_value);
-        success = true;
-        break;
+    // build value from data
+    ValueSP value_sp(NewScalarValue(m_type));
+
+    uint32_t type_flags = m_type.GetTypeInfo();
+    bool is_signed = (type_flags & eTypeIsSigned) != 0;
+
+    switch (m_byte_size) {
+        value_sp->GetScalar() = (int64_t)(raw_value);
+        value_sp->GetScalar() = (uint64_t)(raw_value);
 
-      case sizeof(uint32_t):
-        if (is_signed)
-          value.GetScalar() = (int32_t)(raw_value & UINT32_MAX);
-        else
-          value.GetScalar() = (uint32_t)(raw_value & UINT32_MAX);
-        success = true;
-        break;
+        value_sp->GetScalar() = (int32_t)(raw_value & UINT32_MAX);
+        value_sp->GetScalar() = (uint32_t)(raw_value & UINT32_MAX);
 
-      case sizeof(uint16_t):
-        if (is_signed)
-          value.GetScalar() = (int16_t)(raw_value & UINT16_MAX);
-        else
-          value.GetScalar() = (uint16_t)(raw_value & UINT16_MAX);
-        success = true;
-        break;
+        value_sp->GetScalar() = (int16_t)(raw_value & UINT16_MAX);
+        value_sp->GetScalar() = (uint16_t)(raw_value & UINT16_MAX);
 
-      case sizeof(uint8_t):
-        if (is_signed)
-          value.GetScalar() = (int8_t)(raw_value & UINT8_MAX);
-        else
-          value.GetScalar() = (uint8_t)(raw_value & UINT8_MAX);
-        success = true;
-        break;
-      }
-    } else if (type_flags & eTypeIsFloat) {
-      if (type_flags & eTypeIsComplex) {
-        // Don't handle complex yet.
-      } else {
-        const size_t byte_size = return_compiler_type.GetByteSize(nullptr);
-        if (byte_size <= sizeof(long double)) {
-          const RegisterInfo *f1_info = reg_ctx->GetRegisterInfoByName("f1", 0);
-          RegisterValue f1_value;
-          if (reg_ctx->ReadRegister(f1_info, f1_value)) {
-            DataExtractor data;
-            if (f1_value.GetData(data)) {
-              lldb::offset_t offset = 0;
-              if (byte_size == sizeof(float)) {
-                value.GetScalar() = (float)data.GetFloat(&offset);
-                success = true;
-              } else if (byte_size == sizeof(double)) {
-                value.GetScalar() = (double)data.GetDouble(&offset);
-                success = true;
-              }
-            }
-          }
-        }
+        value_sp->GetScalar() = (int8_t)(raw_value & UINT8_MAX);
+        value_sp->GetScalar() = (uint8_t)(raw_value & UINT8_MAX);
+      break;
+
+    default:
+      llvm_unreachable("Invalid integer size");
+    }
+
+    return value_sp;
+  }
+
+  // get a floating point value on the specified register
+  ValueSP GetFloatValue(CompilerType &type, uint32_t reg_index) {
+    uint64_t raw_data;
+    auto reg = GetFPR(reg_index);
+    if (!reg.GetRawData(raw_data))
+      return ValueSP();
+
+    // build value from data
+    ValueSP value_sp(NewScalarValue(type));
+
+    DataExtractor de(&raw_data, sizeof(raw_data), m_byte_order, m_addr_size);
+
+    offset_t offset = 0;
+    size_t byte_size = type.GetByteSize(nullptr);
+    switch (byte_size) {
+    case sizeof(float):
+      value_sp->GetScalar() = (float)de.GetDouble(&offset);
+      break;
+
+    case sizeof(double):
+      value_sp->GetScalar() = de.GetDouble(&offset);
+
+    default:
+      llvm_unreachable("Invalid floating point size");
+
+    return value_sp;
+
+  // get pointer value from register
+  ValueSP GetPointerValue(uint32_t reg_index) {
+    uint64_t raw_data;
+    auto reg = GetGPR(reg_index);
+    if (!reg.GetRawData(raw_data))
+      return ValueSP();
+
+    // build value from raw data
+    ValueSP value_sp(NewScalarValue(m_type));
+    value_sp->GetScalar() = raw_data;
+    return value_sp;
+
+  // build the ValueObject from our data buffer
+  ValueObjectSP BuildValueObject() {
+    DataExtractor de(DataBufferSP(m_data_ap.release()), m_byte_order,
+                     m_addr_size);
+    return ValueObjectConstResult::Create(&m_thread, m_type, ConstString(""),
+                                          de);
+
+  // get a vector return value
+  ValueObjectSP GetVectorValueObject() {
+    const uint32_t MAX_VRS = 2;
+
+    // get first V register used to return values
+    const RegisterInfo *vr[MAX_VRS];
+    vr[0] = m_reg_ctx->GetRegisterInfoByName("vr2");
+    if (!vr[0]) {
+      LLDB_LOG(m_log, LOG_PREFIX "Failed to get vr2 RegisterInfo");
+      return ValueObjectSP();
+    }
+
+    const uint32_t vr_size = vr[0]->byte_size;
+    size_t vrs = 1;
+    if (m_byte_size > 2 * vr_size) {
+      LLDB_LOG(
+          m_log, LOG_PREFIX
+          "Returning vectors that don't fit in 2 VR regs is not supported");
+      return ValueObjectSP();
+
+    // load vr3, if needed
+    if (m_byte_size > vr_size) {
+      vrs++;
+      vr[1] = m_reg_ctx->GetRegisterInfoByName("vr3");
+      if (!vr[1]) {
+        LLDB_LOG(m_log, LOG_PREFIX "Failed to get vr3 RegisterInfo");
+        return ValueObjectSP();
       }
     }
 
-    if (success)
-      return_valobj_sp = ValueObjectConstResult::Create(
-          thread.GetStackFrameAtIndex(0).get(), value, ConstString(""));
-  } else if (type_flags & eTypeIsPointer) {
-    unsigned r3_id =
-        reg_ctx->GetRegisterInfoByName("r3", 0)->kinds[eRegisterKindLLDB];
-    value.GetScalar() =
-        (uint64_t)thread.GetRegisterContext()->ReadRegisterAsUnsigned(r3_id, 0);
-    value.SetValueType(Value::eValueTypeScalar);
-    return_valobj_sp = ValueObjectConstResult::Create(
-        thread.GetStackFrameAtIndex(0).get(), value, ConstString(""));
-  } else if (type_flags & eTypeIsVector) {
-    const size_t byte_size = return_compiler_type.GetByteSize(nullptr);
-    if (byte_size > 0) {
-      const RegisterInfo *altivec_reg =
-          reg_ctx->GetRegisterInfoByName("vr2",  0);
-      if (altivec_reg) {
-        if (byte_size <= altivec_reg->byte_size) {
-          ProcessSP process_sp(thread.GetProcess());
-          if (process_sp) {
-            std::unique_ptr<DataBufferHeap> heap_data_ap(
-                new DataBufferHeap(byte_size, 0));
-            const ByteOrder byte_order = process_sp->GetByteOrder();
-            RegisterValue reg_value;
-            if (reg_ctx->ReadRegister(altivec_reg, reg_value)) {
-              Status error;
-              if (reg_value.GetAsMemoryData(
-                      altivec_reg, heap_data_ap->GetBytes(),
-                      heap_data_ap->GetByteSize(), byte_order, error)) {
-                DataExtractor data(DataBufferSP(heap_data_ap.release()),
-                                   byte_order, process_sp->GetTarget()
-                                                   .GetArchitecture()
-                                                   .GetAddressByteSize());
-                return_valobj_sp = ValueObjectConstResult::Create(
-                    &thread, return_compiler_type, ConstString(""), data);
-              }
-            }
-          }
-        }
-      }
-    }
+    // Get the whole contents of vector registers and let the
+    // logic here arrange the data properly.
+
+    RegisterValue vr_val[MAX_VRS];
+    std::unique_ptr<DataBufferHeap> vr_data(
+        new DataBufferHeap(vrs * vr_size, 0));
+
+    for (uint32_t i = 0; i < vrs; i++) {
+      if (!m_reg_ctx->ReadRegister(vr[i], vr_val[i])) {
+        LLDB_LOG(m_log, LOG_PREFIX "Failed to read vector register contents");
+        return ValueObjectSP();
+      }
+      if (!vr_val[i].GetAsMemoryData(vr[i], vr_data->GetBytes() + i * vr_size,
+                                     vr_size, m_byte_order, error)) {
+        LLDB_LOG(m_log, LOG_PREFIX "Failed to extract vector register bytes");
+        return ValueObjectSP();
+
+    // The compiler generated code seems to always put the vector elements
+    // at the end of the vector register, in case they don't occupy all of it.
+    // This offset variable handles this.
+    uint32_t offs = 0;
+    if (m_byte_size < vr_size)
+      offs = vr_size - m_byte_size;
+
+    // copy extracted data to our buffer
+    memcpy(m_data_ap->GetBytes(), vr_data->GetBytes() + offs, m_byte_size);
+    return BuildValueObject();
+  }
+
+  // get a struct return value
+  ValueObjectSP GetStructValueObject() {
+    // case 1: get from stack
+    if (m_byte_size > 2 * sizeof(uint64_t)) {
+      uint64_t addr;
+      auto reg = GetGPR(0);
+      if (!reg.GetRawData(addr))
+        return ValueObjectSP();
+
+      Status error;
+      size_t rc = m_process_sp->ReadMemory(addr, m_data_ap->GetBytes(),
+                                           m_byte_size, error);
+      if (rc != m_byte_size) {
+        LLDB_LOG(m_log, LOG_PREFIX "Failed to read memory pointed by r3");
+        return ValueObjectSP();
+      }
+      return BuildValueObject();
+    }
+
+    // get number of children
+    const bool omit_empty_base_classes = true;
+    uint32_t n = m_type.GetNumChildren(omit_empty_base_classes);
+    if (!n) {
+      LLDB_LOG(m_log, LOG_PREFIX "No children found in struct");
+      return ValueObjectSP();
+    }
+
+    // case 2: homogeneous double or float aggregate
+    CompilerType elem_type;
+    if (m_type.IsHomogeneousAggregate(&elem_type)) {
+      uint32_t type_flags = elem_type.GetTypeInfo();
+      uint64_t elem_size = elem_type.GetByteSize(nullptr);
+      if (type_flags & eTypeIsComplex || !(type_flags & eTypeIsFloat)) {
+        LLDB_LOG(m_log,
+                 LOG_PREFIX "Unexpected type found in homogeneous aggregate");
+        return ValueObjectSP();
+      }
+
+      for (uint32_t i = 0; i < n; i++) {
+        ValueSP val_sp = GetFloatValue(elem_type, i);
+        if (!val_sp)
+          return ValueObjectSP();
+
+        // copy to buffer
+        Status error;
+        size_t rc = val_sp->GetScalar().GetAsMemoryData(
+            m_data_ap->GetBytes() + m_dst_offs, elem_size, m_byte_order, error);
+        if (rc != elem_size) {
+          LLDB_LOG(m_log, LOG_PREFIX "Failed to get float data");
+          return ValueObjectSP();
+        }
+        m_dst_offs += elem_size;
+      }
+      return BuildValueObject();
+    }
+
+    // case 3: get from GPRs
+
+    // first, check if this is a packed struct or not
+    ClangASTContext *ast =
+        llvm::dyn_cast<ClangASTContext>(m_type.GetTypeSystem());
+    if (ast) {
+      clang::RecordDecl *record_decl = ClangASTContext::GetAsRecordDecl(m_type);
+
+      if (record_decl) {
+        auto attrs = record_decl->attrs();
+        for (const auto &attr : attrs) {
+          if (attr->getKind() == clang::attr::Packed) {
+            m_packed = true;
+            break;
+          }
+        }
+      }
+    }
+
+    LLDB_LOG(m_log, LOG_PREFIX "{0} struct",
+             m_packed ? "packed" : "not packed");
+
+    for (uint32_t i = 0; i < n; i++) {
+      std::string name;
+      uint32_t size;
+      GetChildType(i, name, size);
+      // NOTE: the offset returned by GetChildCompilerTypeAtIndex()
+      //       can't be used because it never considers alignment bytes
+      //       between struct fields.
+      LLDB_LOG(m_log, LOG_PREFIX "field={0}, size={1}", name, size);
+      if (!ExtractField(size))
+        return ValueObjectSP();
+    }
+
+    return BuildValueObject();
+  }
+
+  // extract 'size' bytes at 'offs' from GPRs
+  bool ExtractFromRegs(int32_t offs, uint32_t size, void *buf) {
+    while (size) {
+      auto reg = GetGPRByOffs(offs);
+      if (!reg.IsValid())
+        return false;
+
+      uint32_t n = std::min(reg.Avail(), size);
+      uint64_t raw_data;
+
+      if (!reg.GetRawData(raw_data))
+        return false;
+
+      memcpy(buf, (char *)&raw_data + reg.Offs(), n);
+      offs += n;
+      size -= n;
+      buf = (char *)buf + n;
+    }
+    return true;
+
+  // extract one field from GPRs and put it in our buffer
+  bool ExtractField(uint32_t size) {
+    auto reg = GetGPRByOffs(m_src_offs);
+    if (!reg.IsValid())
+      return false;
+
+    // handle padding
+    if (!m_packed) {
+      uint32_t n = m_src_offs % size;
+
+      // not 'size' bytes aligned
+      if (n) {
+        LLDB_LOG(m_log,
+                 LOG_PREFIX "Extracting {0} alignment bytes at offset {1}", n,
+                 m_src_offs);
+        // get alignment bytes
+        if (!ExtractFromRegs(m_src_offs, n, m_data_ap->GetBytes() + m_dst_offs))
+          return false;
+        m_src_offs += n;
+        m_dst_offs += n;
+
+    // get field
+    LLDB_LOG(m_log, LOG_PREFIX "Extracting {0} field bytes at offset {1}", size,
+             m_src_offs);
+    if (!ExtractFromRegs(m_src_offs, size, m_data_ap->GetBytes() + m_dst_offs))
+      return false;
+    m_src_offs += size;
+    m_dst_offs += size;
+    return true;
+
+  // get child
+  CompilerType GetChildType(uint32_t i, std::string &name, uint32_t &size) {
+    // GetChild constant inputs
+    const bool transparent_pointers = false;
+    const bool omit_empty_base_classes = true;
+    const bool ignore_array_bounds = false;
+    // GetChild output params
+    int32_t child_offs;
+    uint32_t child_bitfield_bit_size;
+    uint32_t child_bitfield_bit_offset;
+    bool child_is_base_class;
+    bool child_is_deref_of_parent;
+    ValueObject *valobj = nullptr;
+    uint64_t language_flags;
+    ExecutionContext exe_ctx;
+    m_thread.CalculateExecutionContext(exe_ctx);
+
+    return m_type.GetChildCompilerTypeAtIndex(
+        &exe_ctx, i, transparent_pointers, omit_empty_base_classes,
+        ignore_array_bounds, name, size, child_offs, child_bitfield_bit_size,
+        child_bitfield_bit_offset, child_is_base_class,
+        child_is_deref_of_parent, valobj, language_flags);
+  }
+};
+
+#undef LOG_PREFIX
+
+} // anonymous namespace
+
+ValueObjectSP
+ABISysV_ppc64::GetReturnValueObjectSimple(Thread &thread,
+                                          CompilerType &type) const {
+  if (!type)
+    return ValueObjectSP();
+
+  auto exp_extractor = ReturnValueExtractor::Create(thread, type);
+  if (!exp_extractor) {
+    Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+    LLDB_LOG_ERROR(log, exp_extractor.takeError(),
+                   "Extracting return value failed: {0}");
+    return ValueObjectSP();
   }
 
-  return return_valobj_sp;
+  return exp_extractor.get().GetValue();
 }
 
-ValueObjectSP ABISysV_ppc64::GetReturnValueObjectImpl(Thread &thread,
-    CompilerType &return_compiler_type) const {
+ValueObjectSP ABISysV_ppc64::GetReturnValueObjectImpl(
+    Thread &thread, CompilerType &return_compiler_type) const {
   return GetReturnValueObjectSimple(thread, return_compiler_type);
 }
 
 bool ABISysV_ppc64::CreateFunctionEntryUnwindPlan(UnwindPlan &unwind_plan) {
   unwind_plan.Clear();
   unwind_plan.SetRegisterKind(eRegisterKindDWARF);
 
   uint32_t lr_reg_num;
bool ABISysV_ppc64::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
   unwind_plan.Clear();
   unwind_plan.SetRegisterKind(eRegisterKindDWARF);
 
   uint32_t sp_reg_num;
   uint32_t pc_reg_num;
   uint32_t cr_reg_num;
 
   if (GetByteOrder() == lldb::eByteOrderLittle) {
-     sp_reg_num = ppc64le_dwarf::dwarf_r1_ppc64le;
-     pc_reg_num = ppc64le_dwarf::dwarf_lr_ppc64le;
-     cr_reg_num = ppc64le_dwarf::dwarf_cr_ppc64le;
-   } else {
-     sp_reg_num = ppc64_dwarf::dwarf_r1_ppc64;
-     pc_reg_num = ppc64_dwarf::dwarf_lr_ppc64;
-     cr_reg_num = ppc64_dwarf::dwarf_cr_ppc64;
-   }
 
   UnwindPlan::RowSP row(new UnwindPlan::Row);
   const int32_t ptr_size = 8;
   row->GetCFAValue().SetIsRegisterDereferenced(sp_reg_num);
 
   row->SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * 2, true);
   row->SetRegisterLocationToIsCFAPlusOffset(sp_reg_num, 0, true);
   row->SetRegisterLocationToAtCFAPlusOffset(cr_reg_num, ptr_size, true);