[SR-5930] non-Darwin arm and aarch64 reports supportSwiftError()=true but doesn't implement calling convention

[swift-ci]

Previous ID	SR-5930
Radar	None
Original Reporter	paulmenage (JIRA User)
Type	Bug

Additional Detail from JIRA

| | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Bug | |Assignee | None | |Priority | Medium | md5: 5693caf42260171adc330001f06db6b0

Issue Description:

AArch64TargetLowering::supportSwiftError() unconditionally returns true.

AArch64TargetLowering::CCAssignFnForCall() returns CC_AArch64_AAPCS on non-Darwin and CC_AArch64_DarwinPCS on Darwin.

The default CC_AArch64_AAPCS doesn't have any explicit support for putting SwiftError parameters in a register, so the SwiftError gets assigned to a register or to the stack depending on how many other parameters there are before it. If ends up on the stack, we end up with an out-of-bounds array reference in propagateSwiftErrorVRegs():

• SelectionDAGISel::LowerArguments() doesn't call setCurrentSwiftErrorVReg() with the incoming SwiftError parameter since its opcode is ISD::LOAD rather than ISD::CopyFromReg

• The entry basic block of the function has a use but no definition for the SwiftError

• VRegs ends up empty, so VRegs[0] triggers an assertion

Either AArch64TargetLowering::supportSwiftError() should only return true for Darwin, or the default CC_AArch64_AAPCS should include SwiftError register support

[belkadan]

cc @rjmccall

[swift-ci]

Comment by Paul Menage (JIRA)

Something similar applies to the 32-bit ARM backend - supportSwiftError() returns true, and the calling convention does push the SwiftError parameter into a register, but ARMBaseRegisterInfo::getCalleeSavedRegs() only uses CSR_iOS_SwiftError_SaveList when targetting Darwin.

[rjmccall]

cc aschwaighofer@apple.com (JIRA User)

The only reason swiftcc needs to be OS-specific at all is because we don't want unnecessary friction when calling C functions and, to a lesser degree, when being called by C functions. For example, it would be unfortunate if swiftcc used a different stack alignment or a fundamentally different base CSR set, because we'd have to introduce re-aligning code and/or register saves in a lot of functions.

Disabling the swifterror treatment does not seem like the right approach, but otherwise, I defer to Arnold as to the best way of fixing the targets.

[aschwaighofer]

These are llvm bugs. They have been recently addressed in upstream llvm by:

AArch64:
d6812a1a39fb AArch64: support SwiftCC properly on AAPCS64

5d795908c5e [ARM] Use Swift error registers on non-Darwin targets

I will cherry-pick those commits into swift-llvm.

[swift-ci]

Comment by Paul Menage (JIRA)

5d795908c5e uses the iOS CSR set on non-iOS SwiftError calls, which drops r9 from the set compared to AAPCS and also saves r4-r7 in a different order (although I'm not sure of the significance of that latter difference). Wouldn't it be better to define sets like CSR_AAPCS_SwiftError and CSR_AAPCS_SplitPush_SwiftError that just subtract r8 from their respective super-sets (CSR_AAPCS and CSR_AAPCS_SplitPush) and use those when the convention is AAPCS-based?

[aschwaighofer]

Yes, I have committed:

ARM: Use the proper swifterror CSR list on platforms other than darwin

Noticed by inspection

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314121

which does that. Though not for SplitPush.

[swift-ci]

Comment by Paul Menage (JIRA)

Is there a reason not to do so for SplitPush as well? Or would a SwiftError function never end up with the SplitPush CSR convention?

[aschwaighofer]

No reason, other than that I did not pay attention that we had this extra convention. I am about to fix this ...

[aschwaighofer]

Merged the fixes into swift-4.1-branch which means they should soon show up on the stable branch.