Fix function call handling according to Microsoft Arm64 variadic function call ABI #37
Conversation
Blackhex
force-pushed
the
fix-va-list
branch
2 times, most recently
from
3b18f93 to
2cb2f8a
Compare
Blackhex
force-pushed
the
woarm64
branch
2 times, most recently
from
c26bfab to
266c7a4
Compare
Blackhex
force-pushed
the
fix-va-list
branch
2 times, most recently
from
e65268d to
caa9691
Compare
Blackhex
force-pushed
the
fix-va-list
branch
2 times, most recently
from
0321b24 to
d4e43b1
Compare
|
It looks like a right direction. I suggest to get CI results for the toolchain with ucrt. |
Blackhex
force-pushed
the
fix-va-list
branch
2 times, most recently
from
4049420 to
0d90f62
Compare
Blackhex
force-pushed
the
woarm64
branch
11 times, most recently
from
d8ce338 to
a591924
Compare
| extra_headers="arm_fp16.h arm_neon.h arm_bf16.h arm_acle.h arm_sve.h | ||
| arm_sme.h arm_neon_sve_bridge.h arm_private_fp8.h | ||
| arm_private_neon_types.h | ||
| cross-stdarg.h" |
There was a problem hiding this comment.
all headers start with arm_ prefix and use underscore except new one. It looks like it should be moved to mingw and cygwin targets.
There was a problem hiding this comment.
I thought so as well but the i386 implementation is in beteween the common headers as well. I think it has something to do with the fact that those valist type definitions can be relevant accross targets. I'd keep this to be resolved within the discussion with the maintainers.
There was a problem hiding this comment.
ix86 implementation is very different historically, and I do not see reasons why it should not be moved to cygwin and mingw targets.
| @@ -22,6 +22,10 @@ along with GCC; see the file COPYING3. If not see | |||
|
|
|||
| extern tree mingw_handle_selectany_attribute (tree *, tree, tree, int, bool *); | |||
|
|
|||
| #if defined (TARGET_AARCH64_MS_ABI) | |||
| extern tree aarch64_handle_ms_abi_attribute (tree *, tree, tree, int, bool *); | |||
There was a problem hiding this comment.
it should be called mingw_handle_ms_abi_attribute and declared here without the check.
There was a problem hiding this comment.
It's relevant only to AArch64 MinGW, aarch64_mingw_handle_ms_abi_attribute or mingw_aarch64_handle_ms_abi_attribute then?
There was a problem hiding this comment.
mingw folder contains not architecture specific implementation. Based on implementation it looks it is not relaying on aarch64 directly, otherwise it should be moved out to aarch64. It looks like renaming should be enough.
There was a problem hiding this comment.
Should new files aarch64/winnt.h and aarch64/winnt.cc be created then?
There was a problem hiding this comment.
For me it looks like mingw is right place for current implementation with renaming.
There was a problem hiding this comment.
I believe the architecture specific methods should have the architecture in it's name. That implies that the name can be either aarch64_mingw_handle_ms_abi_attribute or mingw_aarch64_handle_ms_abi_attribute. Can you give me another expample of architecture specific function that has not an achitecture in its name?
There was a problem hiding this comment.
for instance mingw_pe_file_end behaves differently based on configuration for ix86 and aarch64 targets.
There was a problem hiding this comment.
I dont understand your point. The code of mingw_pe_file_end itself is generic so it does not have the prefix. It calls i386_find_on_wrapper_list for the architecure specific sub-logic which proves the logic that architecture specific functions should have the architecture prefix.
Do I understand correctly that you are suggesting to refactor of i386 implementation so the ix86_handle_abi_attribute and aarch64_handle_ms_abi_attribute would be merged, containing #if defined (TARGET_AARCH64_MS_ABI) conditions inside?
There was a problem hiding this comment.
yes, ix86_handle_abi_attribute looks alike and should be moved to mingw with renaming to mingw_handle_abi_attribute.
TARGET_AARCH64_MS_ABI or other configuration could be used to adjust the function for aarch64.
There was a problem hiding this comment.
OK, thank you. Now, I see your point.
| attribute_spec.handler. */ | ||
|
|
||
| tree | ||
| aarch64_handle_ms_abi_attribute (tree *node, tree name, tree, int, |
There was a problem hiding this comment.
it looks like TARGET_AARCH64_MS_ABI is not needed here and it brings common implementation not arch specific which could be reused. It should be renamed to mingw_handle_ms_abi_attribute.
There was a problem hiding this comment.
This is architecture-specific implementation i386 has its own in i386/winnt.cc.
| int arg_count = 0; | ||
| for (tree arg = TYPE_ARG_TYPES (fntype); arg; arg = TREE_CHAIN (arg)) | ||
| { | ||
| if (TREE_VALUE (arg) == void_type_node) |
There was a problem hiding this comment.
Should it not be enough to check only first arg for void_type_node?
| arg_count++; | ||
| } | ||
|
|
||
| return arg_count > 0; |
There was a problem hiding this comment.
arg_count 1 is still variadic?
| return aarch64_ms_variadic_abi (); | ||
| #endif | ||
|
|
||
| if (lookup_attribute ("ms_abi", TYPE_ATTRIBUTES (fntype))) |
There was a problem hiding this comment.
what exactly is happening here?
There was a problem hiding this comment.
This logic was re-used form Zac's branch and verified by comparing it with i386 implementation. I believe it has something to do with the ability to explicitly specify that some function call should follow the Microsoft ABI using the attribute explicity. This might be needed for ensuring ABI compatibility accross hybrid environments like wine, WSL, QEMU, etc. Albeit those changes are not necessary for our use-cases, I'd keep them in the patch so we can initiate discussions with people that can bring better understanding for which exactly this is needed and then we can decide whether we should keep and test this feature or remove.
There was a problem hiding this comment.
I would exclude this change until the moment it is known what is changing/fixing. It could be a separate discussion on the mailing list if needed.
There was a problem hiding this comment.
OK, I can move the relevant changes into a separate commit here and submit it as a separate (dependent) patch to the maiiling list.
| ncrn = pcum->aapcs_ncrn; | ||
| nregs = size / UNITS_PER_WORD; | ||
|
|
||
| if (ncrn < NUM_ARG_REGS) |
There was a problem hiding this comment.
too much branching, i would change to do while(false) with breaks and move bottom branch to the top.
There was a problem hiding this comment.
Yes, I agree this can be written using guard-clauses. My intention was to keep this code as similar to the AAPCS one as possible to help reviewers to see the differences better.
| mingw_handle_selectany_attribute, NULL }, \ | ||
| { "ms_abi", 0, 0, false, true, true, true, \ | ||
| aarch64_handle_ms_abi_attribute, NULL }, \ | ||
| { "ms_abi va_list", 0, 0, false, false, false, false, NULL, NULL } |
There was a problem hiding this comment.
Why is it needed and how it helps without a handler?
| nregs -= pcum->aapcs_stack_words; | ||
| } | ||
|
|
||
| /* Generate load arg to registers intructions. */ |
There was a problem hiding this comment.
| /* Generate load arg to registers intructions. */ | |
| /* Generate load arg to registers instructions. */ |
Just a typo I noticed yesterday
Blackhex
force-pushed
the
woarm64
branch
3 times, most recently
from
0f1f207 to
7b6b916
Compare
Blackhex
force-pushed
the
woarm64
branch
11 times, most recently
from
023a528 to
33bedaf
Compare
Blackhex
force-pushed
the
fix-va-list
branch
from
694bf98 to
8fcc7ae
Compare
Proper implementation of Microsoft Arm64 variadic function call ABI.
Tests will be added within Windows-on-ARM-Experiments/mingw-woarm64-build#221
So far tested by https://github.com/Windows-on-ARM-Experiments/mingw-woarm64-build/actions/runs/12813347930
Resolves:
va_liststructure is corrupted mingw-woarm64-build#70atoi+printfdoes not work when linking withucrtmingw-woarm64-build#192 (TODO: Not verified yet.)Contributes to: