This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add support for __builtin_ms_va_list on aarch64
ClosedPublic

Authored by mstorsjo on Jun 21 2017, 1:27 PM.

Details

Summary

Move builtins from the x86 specific scope into the global scope. Their use is still limited to x86_64 and aarch64 though.

This allows wine on aarch64 to properly handle variadic functions.

This depends on D34474 for llvm.

Diff Detail

Event Timeline

mstorsjo created this revision.Jun 21 2017, 1:27 PM
mstorsjo updated this revision to Diff 105200.Jul 4 2017, 2:14 PM

Did a minor adjustment, to make sure a plain va_arg produces the right kind of output, when targeting windows directly (while the rest of the patch mostly is about supporting the MS ABI while targeting a different OS).

mstorsjo updated this revision to Diff 105254.Jul 5 2017, 4:47 AM
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added reviewers: mgrang, t.p.northover.

Split the patch into two; one for generic handling of va_arg for win/arm64, and one for using the same functionality while targeting other OSes (i.e. for wine) via the __builtin_ms_* intrinsics.

mstorsjo updated this revision to Diff 106489.Jul 13 2017, 12:34 PM
mstorsjo retitled this revision from [RFC] [AArch64] Add support for __builtin_ms_va_list on aarch64 to [AArch64] Add support for __builtin_ms_va_list on aarch64.
mstorsjo edited the summary of this revision. (Show Details)

Added test cases, a minor tweak to semantic error handling, removed the RFC tag, removed the reference to the clang prerequisite that now is committed.

rnk added inline comments.Jul 13 2017, 1:10 PM
include/clang/Basic/BuiltinsAArch64.def
65 ↗(On Diff #106489)

I strongly suspect that Microsoft will never adopt a varargs calling convention that uses a complex, non-char* va_list.

I'm starting to think we should move this to the generic builtin list and make it available everywhere. The semantics are that you can only use __builtin_ms_va_start in ms_abi functions. It always produces a char*-style va_list.

include/clang/Basic/Specifiers.h
239

I think we might prefer to make this non-x86_64 specific. I suspect that this pattern will arise again on ARM32 if anyone goes back there in seriousness. We'll probably want sysv_abi as well as ms_abi, and all the logic should be independent of the ISA: ms_abi is a no-op when the target OS is already Windows, and sysv_abi is a no-op when the target OS isn't Windows.

lib/CodeGen/CGBuiltin.cpp
5303

If we made __builtin_ms_va_start generic, that would also eliminate this duplicate code.

lib/CodeGen/CGCall.cpp
221–222

Unifying the ms_abi CCs would remove the need for this.

lib/Sema/SemaChecking.cpp
3620

Oh dear, how can we keep this simple. I think unifying the CC's would improve things.

lib/Sema/SemaDeclAttr.cpp
4284

Ditto

mstorsjo added inline comments.Jul 13 2017, 1:20 PM
include/clang/Basic/BuiltinsAArch64.def
65 ↗(On Diff #106489)

Yes, that seems probable.

I'm a little weary about making this available anywhere though, since it is coupled with the x86_64/aarch64 specific calling convention for lowering va_start, and there we only support it specifically on those two arches.

include/clang/Basic/Specifiers.h
239

FWIW, we already support ARM32 for windows, and there, varargs are identical to other platforms.

Extending this to all platforms probably also makes sense (although I'm a little weary about how it would work for signaling down to the LLVM IR).

rnk added inline comments.Jul 13 2017, 1:25 PM
include/clang/Basic/BuiltinsAArch64.def
65 ↗(On Diff #106489)

We should have Sema checking that rejects it when not targeting x64 or aarch64. Ultimately the codegen is shared and simple: just call llvm.va.start.

mstorsjo updated this revision to Diff 106690.Jul 14 2017, 12:22 PM
mstorsjo edited the summary of this revision. (Show Details)

Initial attempt at unifying the use of __builtin_ms_va_list and calling conventions between x86_64 and aarch64. This goes with the current (un-unified) version of D34474.

I only hooked up attribute((ms_abi)) on aarch64, I didn't hook up sysv_abi yet, because it's not obvious what it should map to, since aarch64 has got two other calling conventions in addition to the win64 one; standard AAPCS (for ELF systems) and the darwin modification of AAPCS. Since that's not directly needed for the wine usecase, I'm omitting that for now.

mstorsjo updated this revision to Diff 106701.Jul 14 2017, 1:24 PM

Updated for the new version of D34474.

Reid, with D34474 approved, does this look like what you had in mind?

rnk accepted this revision.Jul 17 2017, 12:53 PM

Thanks, looks great!

This revision is now accepted and ready to land.Jul 17 2017, 12:53 PM
This revision was automatically updated to reflect the committed changes.