Skip to content

implement VaArgSafe for f128 - #161424

Open
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:va-arg-f128
Open

implement VaArgSafe for f128#161424
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:va-arg-f128

Conversation

@folkertdev

@folkertdev folkertdev commented Aug 20, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

On some platforms, especially when long double is IEEE f128 on the platform.

@folkertdev folkertdev added the F-c_variadic `#![feature(c_variadic)]` label Aug 20, 2026
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2026
@folkertdev

folkertdev commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

@bors try jobs=test-various,aarch64-apple-,-gnu-nopt-,x86_64-mingw-,aarch64-msvc-*,arm-android

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Aug 20, 2026
@bors
Auto merge of #161424 - folkertdev:va-arg-f128, r=<try>
c9719b5 
implement `VaArgSafe` for `f128`


try-job: test-various
try-job: aarch64-apple-*
try-job: *-gnu-nopt-*
try-job: x86_64-mingw-*
try-job: aarch64-msvc-*
try-job: arm-android
folkertdev
folkertdev commented Aug 20, 2026
View reviewed changes

@folkertdev folkertdev left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

r? tgross35

View changes since this review

Comment thread library/core/src/ffi/va_list.rs
Comment thread tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs
@folkertdev
folkertdev marked this pull request as ready for review August 20, 2026 21:52
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2026
@rust-bors

rust-bors Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

💔 Test for c9719b5 failed: CI. Failed job:

@folkertdev

folkertdev commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@bors try jobs=test-various,aarch64-apple-,-gnu-nopt-,x86_64-mingw-,aarch64-msvc-*,arm-android

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Aug 21, 2026
@bors
Auto merge of #161424 - folkertdev:va-arg-f128, r=<try>
d5f25c2 
implement `VaArgSafe` for `f128`


try-job: test-various
try-job: aarch64-apple-*
try-job: *-gnu-nopt-*
try-job: x86_64-mingw-*
try-job: aarch64-msvc-*
try-job: arm-android
folkertdev
folkertdev commented Aug 21, 2026
View reviewed changes
Comment thread library/core/src/ffi/va_list.rs
cfg_select! {
any(
all(target_arch = "x86_64", not(target_vendor = "apple"), not(target_env = "msvc")),
all(target_arch = "x86", not(target_vendor = "apple"), not(target_env = "msvc")),

@folkertdev folkertdev Aug 21, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

clang 23 runs into an alignment bug llvm/llvm-project#217747, but GCC can handle this.

View changes since the review

@rust-bors

rust-bors Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: d5f25c2 (d5f25c2d0c850c37c152ae4a21b215d1c6f7960a)
Base parent: a872286 (a872286d0a1873caec0291ed4304de3170adbe16)

@folkertdev

folkertdev commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 22, 2026
tgross35
tgross35 reviewed Aug 25, 2026
View reviewed changes
Comment thread library/core/src/ffi/va_list.rs
Comment on lines +418 to +446
cfg_select! {
any(
all(target_arch = "x86_64", not(target_vendor = "apple"), not(target_env = "msvc")),
all(target_arch = "x86", not(target_vendor = "apple"), not(target_env = "msvc")),
// BE powerpc requires vsx.
all(target_arch = "powerpc64", target_endian = "little"),
all(
not(windows),
not(target_vendor = "apple"),
any(
target_arch = "aarch64",
target_arch = "loongarch64",
target_arch = "mips64",
target_arch = "mips64r6",
target_arch = "riscv32",
target_arch = "riscv64",
target_arch = "s390x",
target_arch = "sparc64",
target_arch = "wasm32",
target_arch = "wasm64",
),
),
) => {
#[unstable_feature_bound(f128)]
#[unstable(feature = "f128", issue = "116909")]
unsafe impl VaArgSafe for f128 {}
}
_ => { /* unsupported */ }
}

@tgross35 tgross35 Aug 25, 2026 •
edited by rustbot
Loading

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.

This is pretty sparse on background information and I'm not positive what I should be checking against, could you add a comment similar to what __int128 has?

View changes since the review

@folkertdev folkertdev Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one is definitely less principled. Looking into it, __float128 is not a good heuristic based on https://gcc.gnu.org/onlinedocs/gcc-14.3.0/gcc.pdf:

__float128 is available on i386, x86 64, IA-64, LoongArch and hppa HP-UX, as well
as on PowerPC GNU/Linux targets that enable the vector scalar (VSX) instruction set.

Something like defined(__LDBL_MANT_DIG__) && __LDBL_MANT_DIG__ == 113 is also imperfect.

What I'm going for is "anywhere c_longdouble is IEEE f128, and whatever else works".

@folkertdev folkertdev Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added some comments to the va_list.rs definition.

@rustbot

This comment has been minimized.

Sign in to view
This was referenced Aug 28, 2026
Open
Merged
beetrees
beetrees suggested changes Sep 3, 2026
View reviewed changes
Comment thread library/core/src/ffi/va_list.rs Outdated
Comment thread tests/run-make/c-link-to-rust-va-list-fn/test.c Outdated
Comment thread library/core/src/ffi/va_list.rs
// - clang provides `__float128`
// - `long double` is IEEE f128 on the platform.
cfg_select! {
any(

@beetrees beetrees Sep 3, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably worth adding a comment here telling future developers to keep this cfg in sync with the cfgs in the tests.

View changes since the review

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@rustbot

rustbot commented Sep 4, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgross35 tgross35 tgross35 left review comments
+1 more reviewer
@beetrees beetrees beetrees requested changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@tgross35 tgross35

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs F-c_variadic `#![feature(c_variadic)]` 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@folkertdev @rust-log-analyzer @rustbot @tgross35 @beetrees