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

[Leandro Lupori via Phabricator via llvm-commits]

luporl 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;
----------------
clayborg wrote:
> 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();
> ```
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();
```


================
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),
----------------
clayborg wrote:
> "ProcessSP process_sp" argument isn't needed as you can call "thread.GetProcess()" to get it.
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?


https://reviews.llvm.org/D42468