c-variadic: make va_arg match on Arch exhaustive
#150831
Open
+64
−24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-LLVM
Collaborator
|
|
folkertdev
commented
Jan 8, 2026
Comment on lines
-1076
to
+1098
| Arch::LoongArch32 => emit_ptr_va_arg( | ||
| Arch::RiscV32 if target.abi == Abi::Ilp32e => { | ||
| // FIXME: clang manually adjusts the alignment for this ABI. It notes: | ||
| // | ||
| // > To be compatible with GCC's behaviors, we force arguments with | ||
| // > 2×XLEN-bit alignment and size at most 2×XLEN bits like `long long`, | ||
| // > `unsigned long long` and `double` to have 4-byte alignment. This | ||
| // > behavior may be changed when RV32E/ILP32E is ratified. | ||
| bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx)) | ||
| } | ||
| Arch::RiscV32 | Arch::LoongArch32 => emit_ptr_va_arg( |
Contributor
Author
There was a problem hiding this comment.
Comment on lines
+1178
to
+1186
| Arch::Sparc64 => emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| if target_ty_size > 2 * 8 { PassMode::Indirect } else { PassMode::Direct }, | ||
| SlotSize::Bytes8, | ||
| AllowHigherAlign::Yes, | ||
| ForceRightAdjust::No, | ||
| ), |
Contributor
Author
There was a problem hiding this comment.
| Arch::SpirV => bug!("spirv does not support c-variadic functions"), | ||
|
|
||
| Arch::Mips | Arch::Mips32r6 | Arch::Mips64 | Arch::Mips64r6 => { | ||
| // FIXME: port MipsTargetLowering::lowerVAARG. |
Contributor
Author
There was a problem hiding this comment.
The implementation is at https://github.com/llvm/llvm-project/blob/289a3292be0c6a3df86bcdf5be7dd05b79a5570c/llvm/lib/Target/Mips/MipsISelLowering.cpp#L2338
I suspect that is ultimately just emitVoidPtrVAArg but it's hard to tell.
Comment on lines
+1195
to
+1198
| Arch::Sparc | Arch::Avr | Arch::M68k | Arch::Msp430 => { | ||
| // Clang uses the LLVM implementation for these architectures. | ||
| bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx)) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
That implementation is via DefaultABIInfo::EmitVAArg
To EmitVAArgInstr
To the LLVM CreateVAArg I believe this will eventually call expandVAArg
That really looks like emitVoidPtrVAArg to me, but it's hard to be completely sure.
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
more-va-arg-2
branch
from
f6f9040 to
b388d92
Compare
rustbot
added
S-waiting-on-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tracking issue: #44930
Continuing from #150094, the more annoying cases remain. These are mostly very niche targets without Clang
va_argimplementations, and so it might just be easier to defer to LLVM instead of us getting the ABI subtly wrong. That does mean we cannot stabilize c-variadic on those targets I think.Alternatively we could ask target maintainers to contribute an implementation. I'd honestly prefer they make that change to LVM though (likely by just using
CodeGen::emitVoidPtrVAArg) that we can mirror.r? @workingjubilee