-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
support c-variadic functions in rustc_const_eval
#150601
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
d30044f to
5f98625
Compare
This comment has been minimized.
This comment has been minimized.
5f98625 to
3368321
Compare
This comment has been minimized.
This comment has been minimized.
3368321 to
41a34bc
Compare
This comment has been minimized.
This comment has been minimized.
41a34bc to
8608ba7
Compare
This comment has been minimized.
This comment has been minimized.
8608ba7 to
207b032
Compare
|
☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts. |
207b032 to
1794556
Compare
This comment has been minimized.
This comment has been minimized.
b297adc to
c081ba3
Compare
This comment has been minimized.
This comment has been minimized.
c081ba3 to
293ba18
Compare
|
This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred in compiler/rustc_codegen_gcc |
|
☔ The latest upstream changes (presumably #146923) made this pull request unmergeable. Please resolve the merge conflicts. |
this does not detect UB yet when a `VaList` is copied manually
2763963 to
9e57c73
Compare
|
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. |
| /// Map for "extra" function pointers. | ||
| extra_fn_ptr_map: FxIndexMap<AllocId, M::ExtraFnVal>, | ||
|
|
||
| pub(crate) va_list_map: FxIndexMap<AllocId, Vec<MPlaceTy<'tcx, M::Provenance>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, you have a separate map here to give "meaning" to a VaList allocation... that's the part I missed earlier.
If you have that... why do you even need the new variant in GlobalAlloc? I know I told you to add that, but I also didn't expect a va_list_map field. I like the idea with the field, but it's not clear to me that we need both the field and the new kind of GlobalAlloc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An allocation is still needed to make the pointer. None of the other variants fit nicely I think, but maybe I'm just missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A GlobalAlloc is only needed for global allocations, as the name suggests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how should I get an AllocId then? something like this?
let (alloc_id, _, _) = {
let f = self.layout_of(self.tcx.types.unit).unwrap();
let mplace = MPlaceTy::fake_alloc_zst(f);
let ptr = mplace.ptr();
self.ptr_get_alloc_id(ptr, 0)?
};this specifically causes
pointer not dereferenceable: pointer must point to some allocation, but got 0x1[noalloc] which is a dangling pointer (it has no provenance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other approaches frustratingly create a Pointer instead of a Pointer:<M::Provemance>...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your new va_list_map works basically like the existing extra_fn_ptr_map, so you can copy what we do for that:
rust/compiler/rustc_const_eval/src/interpret/memory.rs
Lines 221 to 222 in 7ec34de
| let id = self.tcx.reserve_alloc_id(); | |
| let old = self.memory.extra_fn_ptr_map.insert(id, extra); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please keep va_list_map private to this file and have methods similar to the existing special allocation kinds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the above, you still get a Pointer<CmseProvenance>. Other code uses global_root_pointer to turn that into a Pointer<M::Provenance>, but that fails when the pointer is not actually a global allocation. Function pointers would still use
pub enum GlobalAlloc<'tcx> {
/// The alloc ID is used as a function pointer.
Function { instance: Instance<'tcx> },
// ...
}I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I found the assert that needs to change, nvm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will have to add a new variant to this enum and take that into account in Miri.
a3a75ad to
19347f2
Compare
|
@rustbot author (I haven't yet had a chance to look at the code in detail, but given the larger structural issues it's probably better tog et those right before I do the detailed review.) |
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
19347f2 to
83db12a
Compare
| // Store the pointer to the global variable arguments list allocation in the | ||
| // first bytes of the `VaList` value. | ||
| let mut alloc = self | ||
| .get_ptr_alloc_mut(mplace.ptr(), self.data_layout().pointer_size())? | ||
| .expect("not a ZST"); | ||
|
|
||
| alloc.write_ptr_sized(Size::ZERO, addr)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for const evaluation, but with Miri when I read this pointer value again in va_arg or va_endit has no provenance. Should I be storing the pointer in some other way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push the code and tests? CI seems happy.
| }; | ||
|
|
||
| let (prov, _offset) = va_list_ptr.into_raw_parts(); | ||
| let alloc_id = prov.unwrap().get_alloc_id().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in any case, the current example will fail here. The prov.unwrap() errors out:
Backtrace
error: no output was expected
Execute `./miri test --bless` to update `tests/pass/c-variadic.stderr` to the actual output
+++ <stderr output>
thread 'rustc' (330406) panicked at /home/folkertdev/rust/rust/compiler/rustc_const_eval/src/interpret/intrinsics.rs:788:37:
called `Option::unwrap()` on a `None` value
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: core::panicking::panic
3: core::option::unwrap_failed
4: <rustc_const_eval::interpret::eval_context::InterpCx<miri::machine::MiriMachine>>::eval_intrinsic
5: <rustc_const_eval::interpret::eval_context::InterpCx<miri::machine::MiriMachine>>::init_fn_call
6: <rustc_const_eval::interpret::eval_context::InterpCx<miri::machine::MiriMachine> as miri::concurrency::thread::EvalContextExt>::run_threads
7: miri::eval::eval_entry
8: <miri::MiriCompilerCalls as rustc_driver_impl::Callbacks>::after_analysis
9: <std::thread::local::LocalKey<core::cell::Cell<*const ()>>>::with::<rustc_middle::ty::context::tls::enter_context<<rustc_middle::ty::context::GlobalCtxt>::enter<rustc_interface::passes::create_and_enter_global_ctxt<core::option::Option<rustc_interface::queries::Linker>, rustc_driver_impl::run_compiler::{closure#0}::{closure#2}>::{closure#2}::{closure#0}, core::option::Option<rustc_interface::queries::Linker>>::{closure#1}, core::option::Option<rustc_interface::queries::Linker>>::{closure#0}, core::option::Option<rustc_interface::queries::Linker>>
10: <rustc_middle::ty::context::TyCtxt>::create_global_ctxt::<core::option::Option<rustc_interface::queries::Linker>, rustc_interface::passes::create_and_enter_global_ctxt<core::option::Option<rustc_interface::queries::Linker>, rustc_driver_impl::run_compiler::{closure#0}::{closure#2}>::{closure#2}::{closure#0}>
11: <rustc_interface::passes::create_and_enter_global_ctxt<core::option::Option<rustc_interface::queries::Linker>, rustc_driver_impl::run_compiler::{closure#0}::{closure#2}>::{closure#2} as core::ops::function::FnOnce<(&rustc_session::session::Session, rustc_middle::ty::context::CurrentGcx, alloc::sync::Arc<rustc_data_structures::jobserver::Proxy>, &std::sync::once_lock::OnceLock<rustc_middle::ty::context::GlobalCtxt>, &rustc_data_structures::sync::worker_local::WorkerLocal<rustc_middle::arena::Arena>, &rustc_data_structures::sync::worker_local::WorkerLocal<rustc_hir::Arena>, rustc_driver_impl::run_compiler::{closure#0}::{closure#2})>>::call_once::{shim:vtable#0}
12: rustc_interface::passes::create_and_enter_global_ctxt::<core::option::Option<rustc_interface::queries::Linker>, rustc_driver_impl::run_compiler::{closure#0}::{closure#2}>
13: rustc_interface::interface::run_compiler::<(), rustc_driver_impl::run_compiler::{closure#0}>::{closure#1}
14: <scoped_tls::ScopedKey<rustc_span::SessionGlobals>>::set::<rustc_interface::util::run_in_thread_with_globals<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<(), rustc_driver_impl::run_compiler::{closure#0}>::{closure#1}, ()>::{closure#0}, ()>::{closure#0}::{closure#0}::{closure#0}, ()>
15: rustc_span::create_session_globals_then::<(), rustc_interface::util::run_in_thread_with_globals<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<(), rustc_driver_impl::run_compiler::{closure#0}>::{closure#1}, ()>::{closure#0}, ()>::{closure#0}::{closure#0}::{closure#0}>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/miri/issues/new
note: please make sure that you have updated to the latest nightly
note: please attach the file at `/home/folkertdev/rust/rust/src/tools/miri/rustc-ice-2026-01-22T18_30_49-330404.txt` to your bug report
note: rustc 1.95.0-dev running on x86_64-unknown-linux-gnu
note: compiler flags: -Z ui-testing
query stack during panic:
end of query stack
Miri caused an ICE during evaluation. Here's the interpreter backtrace at the time of the panic:
note: the place in the program where the ICE was triggered
--> /home/folkertdev/rust/rust/library/core/src/ffi/va_list.rs:224:18
|
LL | unsafe { va_end(self) }
| ^^^^^^^^^^^^
|
= note: stack backtrace:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally seems somewhat hacky, using into_raw_parts isn't great.
If you are creating a new allocation on each va_arg call anyway, why do you store the "list index" in the offset? That just poses the risk that someone does add to change where in the list we are. Shouldn't it also work to have just the remaining elements in the va_list_map and have the pointer always point at the "beginning" of the allocation? Then you can define a helper similar to get_ptr_fn that takes a pointer and returns the remaining va_arg list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest commit the offset is no longer used. That does not solve the issue though: provenance on the pointer is lost when it is written to and then read again from memory.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
tracking issue: #44930
The new
GlobalAlloc::VaListis used to create anAllocIdthat represents the variable argument list of a frame. The allocation itself does not store any data, all we need is the unique identifier.The actual variable argument list is stored in
Memory, and keyed by theAllocId. TheFramealso stores thisAllocId, so that when a frame is popped, it can deallocate the variable arguments.At "runtime" a
VaListvalue stores a pointer to the global allocation in its first bytes. The provenance on this pointer can be used to retrieve itsAllocId, and the offset of the pointer is used to store the index of the next argument to read from the variable argument list.Miri does not yet support
va_arg, but I think that can be done separetely?r? @RalfJung
cc @workingjubilee