Skip to content

remove LLVM va_end calls#157627

Open
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:remove-llvm-va_end
Open

remove LLVM va_end calls#157627
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:remove-llvm-va_end

Conversation

@folkertdev

Copy link
Copy Markdown
Contributor

tracking issue: #44930

The operation is a no-op, so we skip it.

The operation is a no-op, so we skip it.
@folkertdev folkertdev added the F-c_variadic `#![feature(c_variadic)]` label Jun 8, 2026
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. labels Jun 8, 2026
@folkertdev

Copy link
Copy Markdown
Contributor Author

r? RalfJung

The changes are trivial, but technically we're introducing LLVM UB here, so passing this to you for signoff.

We also don't use LLVM's va_copy, but I think that is meaningfully different because we never call LLVM's va_end or va_arg on that, hence LLVM has no way of observing this at all.

But LLVM can actually see that we emit a va_start without a corresponding va_end, and based on its documentation that is UB and it's allowed to do whatever.

@rustbot

rustbot commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@folkertdev folkertdev marked this pull request as ready for review June 8, 2026 21:49
@rustbot

rustbot commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

@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 Jun 8, 2026
@RalfJung

RalfJung commented Jun 9, 2026

Copy link
Copy Markdown
Member

LLVM can also see what we memcpy a VaList and then call va_arg on both the old and new value which is UB (because it's not documented to be allowed). So if we want to not have LLVM UB, we need to actually emit va_copy / va_end for copying / dropping all VaList.

@nikic what do you think?

@RalfJung RalfJung Jun 9, 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.

What was the reason again for us not calling va_copy / va_end when cloning / dropping a VaList?

View changes since the review

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.

Well another constraint on the LLVM va_end is that it must be called in the same function as the corresponding va_start or va_copy, but also va_list can be moved. So va_end does not actually correspond to rust Drop.

@folkertdev folkertdev left a comment

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.

LLVM can also see what we memcpy a VaList and then call va_arg on both the old and new value which is UB (because it's not documented to be allowed).

It feels different, we're not using LLVM's va_arg but our own thing, but it does modify the VaList so I guess that counts.

View changes since this review

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.

Well another constraint on the LLVM va_end is that it must be called in the same function as the corresponding va_start or va_copy, but also va_list can be moved. So va_end does not actually correspond to rust Drop.

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

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants