[PATCH] D42468: [lldb][PPC64] Fixed vector and struct return value

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp:449
 
-  // value.SetContext (Value::eContextTypeClangType, return_value_type);
-  value.SetCompilerType(return_compiler_type);
+    std::string GetGPRName() const {
+      std::string regName;
----------------
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();
```


================
Comment at: source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp:457
 
-  RegisterContext *reg_ctx = thread.GetRegisterContext().get();
-  if (!reg_ctx)
-    return return_valobj_sp;
-
-  const uint32_t type_flags = return_compiler_type.GetTypeInfo();
-  if (type_flags & eTypeIsScalar) {
-    value.SetValueType(Value::eValueTypeScalar);
-
-    bool success = false;
-    if (type_flags & eTypeIsInteger) {
-      // Extract the register context so we can read arguments from registers
-
-      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;
-
-      case sizeof(uint64_t):
-        if (is_signed)
-          value.GetScalar() = (int64_t)(raw_value);
-        else
-          value.GetScalar() = (uint64_t)(raw_value);
-        success = true;
-        break;
-
-      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;
-
-      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;
-
-      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;
+    std::string GetFPRName() const {
+      std::string regName;
----------------
Seems like this function is so similar to GetGPRName, both contents can be inlined into GetName() as noted above?


================
Comment at: source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp:588
+  ReturnValueExtractor(Thread &thread, CompilerType &type,
+                       RegisterContext *reg_ctx, ProcessSP process_sp)
+      : m_thread(thread), m_type(type),
----------------
"ProcessSP process_sp" argument isn't needed as you can call "thread.GetProcess()" to get it.


https://reviews.llvm.org/D42468