Skip to content

c-variadic: add roundtrip test - #155486

Merged
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
folkertdev:c-variadic-roundtrip
Apr 19, 2026
Merged

c-variadic: add roundtrip test#155486
rust-bors[bot] merged 2 commits into
rust-lang:mainfrom
folkertdev:c-variadic-roundtrip

Conversation

@folkertdev

Copy link
Copy Markdown
Contributor

tracking issue: #44930

Test that our va_arg implementation matches (as in, can decode) how LLVM passes c-variadic arguments.

And some comment followup to #152980 (cc @RalfJung, feel free to review this PR too btw).

r? tgross35

@rustbotrustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

@rust-bors

This comment has been minimized.

Comment threadtests/ui/c-variadic/roundtrip.rs Outdated
use std::ffi::*;

// Test that the rustc implementation of `va_arg` works with the LLVM implementation of passing
// c-variadic arguments.

@RalfJungRalfJungApr 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand this comment. We don't use the LLVM implementation, we have our own, right?

View changes since the review

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We have our own implementation of va_arg for the callee reading from a VaList. But how the caller passes the arguments, and how that VaList is constructed, is determined by LLVM.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah :) We don't have any such tests yet...?!?

(Looking at the tests)

We do have a few things, but not covering all the types one can pass. Makes sense, thanks.

Comment threadtests/ui/c-variadic/roundtrip.rs Outdated
Comment on lines +13 to +14
// Intersperse a small type to test alignment logic.
assert!(ap.arg::<u32>() == 0xAAAA_AAAA);

@RalfJungRalfJungApr 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

4 bytes doesn't seem that small. Why not a 1-byte type?

View changes since the review

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Because of the promotion rules/VaArgSafe, a c_int is the smallest thing we can read.

@folkertdev
folkertdevforce-pushed the c-variadic-roundtrip branch from 77808a5 to 80ffa50CompareApril 19, 2026 10:51
@rustbot

Copy link
Copy Markdown
Collaborator

Some changes occurred in src/tools/cargo

cc @ehuss

@rustbot

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot

This comment has been minimized.

Comment threadsrc/tools/cargo

@RalfJungRalfJungApr 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

something went wrong here

View changes since the review

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yeah when you rebase but don't build git add -u does not play well with submodules... fixed now

@folkertdev
folkertdevforce-pushed the c-variadic-roundtrip branch from 80ffa50 to fa740c7CompareApril 19, 2026 10:55
@RalfJung

Copy link
Copy Markdown
Member

Looks good, thanks :)

@bors r+ rollup

@rust-bors

rust-borsBot commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

📌 Commit fa740c7 has been approved by RalfJung

It is now in the queue for this repository.

@rust-borsrust-borsBot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2026
rust-borsBot pushed a commit that referenced this pull request Apr 19, 2026
…uwer
Rollup of 12 pull requests
Successful merges:
- #155370 (Add regression test for dead code elimination with drop + panic)
- #154823 (Replace the spdx-rs dependency with a minimal in-tree SPDX tag-value parser)
- #155294 (Add test for coalescing of diagnostic attribute duplicates)
- #155352 (triagebot.toml: Sync `assign.owners` with `autolabel."T-compiler"`)
- #155431 (Add temporary scope to assert_matches)
- #153873 (deprecate `std::char` constants and functions)
- #154865 (libtest: use binary search for --exact test filtering)
- #154979 (add #[must_use] macros for floats)
- #155486 (c-variadic: add roundtrip test)
- #155504 (Remove `AttributeLintKind` variants - part 2)
- #155510 (Update Tidy python executable path)
- #155514 (codegen-options docs: remove -Csoft-float)
@rust-bors
rust-borsBot merged commit 0f6fe2e into rust-lang:mainApr 19, 2026
11 checks passed
@rustbotrustbot added this to the 1.97.0 milestone Apr 19, 2026
rust-timer added a commit that referenced this pull request Apr 19, 2026
Rollup merge of #155486 - folkertdev:c-variadic-roundtrip, r=RalfJung
c-variadic: add roundtrip test
tracking issue: #44930
Test that our `va_arg` implementation matches (as in, can decode) how LLVM passes c-variadic arguments.
And some comment followup to #152980 (cc @RalfJung, feel free to review this PR too btw).
r? tgross35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@folkertdev@rustbot@RalfJung@tgross35