Page MenuHomePhabricator

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

Authored by luporl on Jan 24 2018, 4:28 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

luporl created this revision.Jan 24 2018, 4:28 AM
luporl edited the summary of this revision. (Show Details)Jan 24 2018, 4:51 AM
luporl added reviewers: labath, clayborg.
luporl set the repository for this revision to rL LLVM.
luporl added subscribers: alexandreyy, anajuliapc, lbianc.
labath added inline comments.Jan 24 2018, 5:26 AM
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 added inline comments.Jan 24 2018, 6:11 AM
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.

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

luporl updated this revision to Diff 131274.Jan 24 2018, 8:30 AM
  • 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?

luporl marked 3 inline comments as done.Jan 24 2018, 8:33 AM
clayborg added inline comments.Jan 24 2018, 10:14 AM
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 added inline comments.Jan 24 2018, 10:58 AM
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?

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 updated this revision to Diff 132211.EditedJan 31 2018, 10:44 AM

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 accepted this revision.Feb 1 2018, 6:32 AM

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).

This revision is now accepted and ready to land.Feb 1 2018, 6:32 AM

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 updated this revision to Diff 132455.Feb 1 2018, 12:45 PM
  • Made the get register name code shorter.
  • Merge branch 'master' into fix-return-value
luporl marked 8 inline comments as done.Feb 1 2018, 12:49 PM

Is there anything pending on this?

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

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

This revision was automatically updated to reflect the committed changes.