This is an archive of the discontinued LLVM Phabricator instance.

Don't try to get memory returns for ABIMacOSX_arm64
ClosedPublic

Authored by jingham on Mar 9 2022, 5:28 PM.

Details

Summary

The ARM64 ABI does not require restoring the memory return register value (x8) on exit from a function, so lldb can't reliably fetch memory return values. We were still trying and that caused a bunch of tests in TestReturnValue to fail. I changed the ABI code to not try to fetch return values for memory returns. I also changed the tests to assert that for all the cases of memory returns we return no value on Darwin arm64, since we shouldn't be guessing...

Diff Detail

Event Timeline

jingham created this revision.Mar 9 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 5:28 PM
jingham requested review of this revision.Mar 9 2022, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 5:28 PM

Ach, that's unfortunate. So a caller can allocate space for the return object, pass that address to a callee in x8, but when callee returns, x8 has been overwritten. Agree that this is the only right way to behave.

labath added a subscriber: labath.Mar 10 2022, 10:35 AM

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

The ABI docs say "you aren't required to restore the value of x8 on function return"... Not all that surprising everybody took the speed over debuggability option.

JDevlieghere accepted this revision.Mar 10 2022, 12:55 PM

LGMT with the SysV ABI updated as well.

This revision is now accepted and ready to land.Mar 10 2022, 12:55 PM
jingham updated this revision to Diff 414765.Mar 11 2022, 3:48 PM

At Pavel's request I extended this to the SysV_arm64 ABI. Since I don't want to have to guess which arch's are which ABI's, I exposed the ABI plugin's name and then used that in the test.

JDevlieghere added inline comments.Mar 11 2022, 5:08 PM
lldb/source/API/SBTarget.cpp
1596–1597 ↗(On Diff #414765)

I think ConstString has a ctor that takes a StringRef so you should be able to do ConstString abi_name(target_sp->GetABIName()) directly.

labath accepted this revision.Mar 14 2022, 1:39 AM

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

The ABI docs say "you aren't required to restore the value of x8 on function return"... Not all that surprising everybody took the speed over debuggability option.

Yeah, I'm actually pretty fine with this. I dread the day when they start routinely (I think it already happens in a limited fashion) start using ABI's made up on the spot for translation-unit-local functions.

At Pavel's request I extended this to the SysV_arm64 ABI. Since I don't want to have to guess which arch's are which ABI's, I exposed the ABI plugin's name and then used that in the test.

Heh.. one of my reasons for requesting this was to avoid complex logic in the test -- I thought you'd just say that the functionality is not available on all arm64 architectures. Though I suppose having the plugin name around could be useful in general...