Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix varargs support on aarch64-apple-darwin #1500

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

beetrees
Copy link
Contributor

Uses the strategy outlined in #1451 (comment) to enable support for varargs functions with integer/pointer arguments on aarch64-apple-darwin. CI testing is also added for aarch64-apple-darwin.

@beetrees beetrees marked this pull request as ready for review June 18, 2024 21:18
src/abi/mod.rs Outdated
&& fn_sig.c_variadic()
// No need to pad the argument list unless vardiac arguments are actually being passed.
&& args.len() > fixed_arg_count
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of code here. Would it be possible to move it to the if fn_sig.c_variadic() { if statement after emitting the call inst? Basically the idea would be to modify the argument list of the call instruction and splice extra arguments in. You can probably use func.dfg.overwrite_inst_values, taking care to account for the first arg of call_indirect being the function pointer rather than a call arguments. This would keep all vararg hacks in a single place which can easily be removed once Cranelift lands real vararg support.

Copy link
Contributor Author

@beetrees beetrees Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately overwrite_inst_values doesn't allow changing the number of Values. As most of the block in the closure passed to codegen_with_call_return_arg is handling varargs, I've left a comment at the top of it stating which bits aren't hacky workarounds for Cranelift's lack of C varargs support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe match func.stencil.dfg.inst[call_inst] { InstructionData::Call { args } => { ... }, InstructionData::CallIndirect { args, ... } => { ... } with args.insert(index, zero, &mut func.stencil.dfg.value_lists) to actually add the arguments would work? If not what you have right now is fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored all the C varargs handling into a dedicated adjust_call_for_c_variadic function that gets called just before the call instruction is generated.

src/abi/mod.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Member

bjorn3 commented Jun 19, 2024

Thanks a lot for this PR! While this is a big hack, so is the rest of the vararg support in cg_clif, so I don't see it as a reason to reject this PR. I would like to see all parts of the vararg support hack to be placed together though. Also if you don't mind, could you ask a CI job for aarch64-apple-darwin to test it? This should be a matter of duplicating the block at

- os: macos-latest
env:
TARGET_TRIPLE: x86_64-apple-darwin
and adjusting the TARGET_TRIPLE. You can also do the same at
- os: macos-latest
env:
TARGET_TRIPLE: x86_64-apple-darwin
to get CI to upload a prebuilt version of cg_clif for arm64 macOS.

@beetrees beetrees force-pushed the mac-arm64-va-call branch 2 times, most recently from 2d1255e to a0341ee Compare June 19, 2024 15:34
@beetrees
Copy link
Contributor Author

I've added a dist CI job (I'd already added a PR CI job). While testing the implementation, I noticed a pre-existing bug: when fixing the signature of a C variadic function to match its arguments, the implementation used to blanket overwrite all the AbiParams with new ones. This meant that if any of the arguments had ArgumentExtension or ArgumentPurpose, this information would get lost (I noticed this as aarch64 passes StructReturn arguments in a dedicated register). I've changed the code so it only adds the additional argument AbiParams as a temporary fix.

@beetrees beetrees marked this pull request as draft June 19, 2024 16:00
@beetrees beetrees force-pushed the mac-arm64-va-call branch from a0341ee to 1321af5 Compare June 19, 2024 16:50
@beetrees
Copy link
Contributor Author

I've fixed it so it now uses clif_sig_from_fn_abi to calculate the function signature properly, including any varargs.

@beetrees beetrees marked this pull request as ready for review June 19, 2024 16:51
@beetrees beetrees force-pushed the mac-arm64-va-call branch from 1321af5 to ca9381f Compare June 19, 2024 23:05
@beetrees beetrees force-pushed the mac-arm64-va-call branch from ca9381f to b0fcf2e Compare June 19, 2024 23:08
@bjorn3 bjorn3 merged commit cdad523 into rust-lang:master Jun 20, 2024
20 checks passed
@beetrees beetrees deleted the mac-arm64-va-call branch June 20, 2024 11:55
@bjorn3
Copy link
Member

bjorn3 commented Jun 20, 2024

@beetrees I want to post an announcement on Mastodon. Do you want me to credit you as @\beetrees?

@beetrees
Copy link
Contributor Author

Yes, that would be great. Thanks.

@bjorn3
Copy link
Member

bjorn3 commented Jun 20, 2024

https://hachyderm.io/@bjorn3/112648887843960881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants