minor tweaks and comments

6 files changed, 35 insertions, 25 deletions

diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs
index abfee0ee874..878afdf2517 100644
--- a/src/tools/miri/src/concurrency/thread.rs
+++ b/src/tools/miri/src/concurrency/thread.rs
@@ -677,6 +677,8 @@ trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> {
     fn run_on_stack_empty(&mut self) -> InterpResult<'tcx, Poll<()>> {
         let this = self.eval_context_mut();
         // Inform GenMC that a thread has finished all user code. GenMC needs to know this for scheduling.
+        // FIXME(GenMC): Thread-local destructors *are* user code, so this is odd. Also now that we
+        // support pre-main constructors, it can get called there as well.
         if let Some(genmc_ctx) = this.machine.data_race.as_genmc_ref() {
             let thread_id = this.active_thread();
             genmc_ctx.handle_thread_stack_empty(thread_id);
diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs
index d986f2830aa..be6404f64e8 100644
--- a/src/tools/miri/src/eval.rs
+++ b/src/tools/miri/src/eval.rs
@@ -18,7 +18,7 @@ use rustc_session::config::EntryFnType;
 use crate::concurrency::GenmcCtx;
 use crate::concurrency::thread::TlsAllocAction;
 use crate::diagnostics::report_leaks;
-use crate::shims::{ctor, tls};
+use crate::shims::{global_ctor, tls};
 use crate::*;
 
 #[derive(Copy, Clone, Debug)]
@@ -219,9 +219,11 @@ impl Default for MiriConfig {
 #[derive(Debug)]
 enum MainThreadState<'tcx> {
     GlobalCtors {
-        ctor_state: ctor::GlobalCtorState<'tcx>,
+        ctor_state: global_ctor::GlobalCtorState<'tcx>,
+        /// The main function to call.
         entry_id: DefId,
         entry_type: MiriEntryFnType,
+        /// Arguments passed to `main`.
         argc: ImmTy<'tcx>,
         argv: ImmTy<'tcx>,
     },
@@ -324,12 +326,19 @@ pub fn create_ecx<'tcx>(
         MiriMachine::new(config, layout_cx, genmc_ctx),
     );
 
-    // First argument is constructed later, because it's skipped for `miri_start.`
+    // Make sure we have MIR. We check MIR for some stable monomorphic function in libcore.
+    let sentinel =
+        helpers::try_resolve_path(tcx, &["core", "ascii", "escape_default"], Namespace::ValueNS);
+    if !matches!(sentinel, Some(s) if tcx.is_mir_available(s.def.def_id())) {
+        tcx.dcx().fatal(
+            "the current sysroot was built without `-Zalways-encode-mir`, or libcore seems missing. \
+            Use `cargo miri setup` to prepare a sysroot that is suitable for Miri."
+        );
+    }
 
-    // Second argument (argc): length of `config.args`.
+    // Compute argc and argv from `config.args`.
     let argc =
         ImmTy::from_int(i64::try_from(config.args.len()).unwrap(), ecx.machine.layouts.isize);
-    // Third argument (`argv`): created from `config.args`.
     let argv = {
         // Put each argument in memory, collect pointers.
         let mut argvs = Vec::<Immediate<Provenance>>::with_capacity(config.args.len());
@@ -354,7 +363,7 @@ pub fn create_ecx<'tcx>(
             ecx.write_immediate(arg, &place)?;
         }
         ecx.mark_immutable(&argvs_place);
-        // Store `argc` and `argv` for macOS `_NSGetArg{c,v}`.
+        // Store `argc` and `argv` for macOS `_NSGetArg{c,v}`, and for the GC to see them.
         {
             let argc_place =
                 ecx.allocate(ecx.machine.layouts.isize, MiriMemoryKind::Machine.into())?;
@@ -369,7 +378,7 @@ pub fn create_ecx<'tcx>(
             ecx.machine.argv = Some(argv_place.ptr());
         }
         // Store command line as UTF-16 for Windows `GetCommandLineW`.
-        {
+        if tcx.sess.target.os == "windows" {
             // Construct a command string with all the arguments.
             let cmd_utf16: Vec<u16> = args_to_utf16_command_string(config.args.iter());
 
@@ -392,28 +401,20 @@ pub fn create_ecx<'tcx>(
 
     // Some parts of initialization require a full `InterpCx`.
     MiriMachine::late_init(&mut ecx, config, {
-        let mut state = MainThreadState::GlobalCtors {
+        let mut main_thread_state = MainThreadState::GlobalCtors {
             entry_id,
             entry_type,
             argc,
             argv,
-            ctor_state: ctor::GlobalCtorState::default(),
+            ctor_state: global_ctor::GlobalCtorState::default(),
         };
 
         // Cannot capture anything GC-relevant here.
-        Box::new(move |m| state.on_main_stack_empty(m))
+        // `argc` and `argv` *are* GC_relevant, but they also get stored in `machine.argc` and
+        // `machine.argv` so we are good.
+        Box::new(move |m| main_thread_state.on_main_stack_empty(m))
     })?;
 
-    // Make sure we have MIR. We check MIR for some stable monomorphic function in libcore.
-    let sentinel =
-        helpers::try_resolve_path(tcx, &["core", "ascii", "escape_default"], Namespace::ValueNS);
-    if !matches!(sentinel, Some(s) if tcx.is_mir_available(s.def.def_id())) {
-        tcx.dcx().fatal(
-            "the current sysroot was built without `-Zalways-encode-mir`, or libcore seems missing. \
-            Use `cargo miri setup` to prepare a sysroot that is suitable for Miri."
-        );
-    }
-
     interp_ok(ecx)
 }
 
diff --git a/src/tools/miri/src/shims/ctor.rs b/src/tools/miri/src/shims/global_ctor.rs
index 367c2b1608b..c56251bbe63 100644
--- a/src/tools/miri/src/shims/ctor.rs
+++ b/src/tools/miri/src/shims/global_ctor.rs
@@ -45,8 +45,12 @@ impl<'tcx> GlobalCtorState<'tcx> {
 
                                 segment_name == Some("__DATA")
                                     && section_name == Some("__mod_init_func")
-                                    // The `mod_init_funcs` directive ensures that the `S_MOD_INIT_FUNC_POINTERS` flag
-                                    // is set on the section, but it is not strictly required.
+                                    // The `mod_init_funcs` directive ensures that the
+                                    // `S_MOD_INIT_FUNC_POINTERS` flag is set on the section. LLVM
+                                    // adds this automatically so we currently do not require it.
+                                    // FIXME: is this guaranteed LLVM behavior? If not, we shouldn't
+                                    // implicitly add it here. Also see
+                                    // <https://github.com/rust-lang/miri/pull/4459#discussion_r2200115629>.
                                     && matches!(section_type, None | Some("mod_init_funcs"))
                             })?,
 
diff --git a/src/tools/miri/src/shims/mod.rs b/src/tools/miri/src/shims/mod.rs
index 0faf298bfaa..75540f6f150 100644
--- a/src/tools/miri/src/shims/mod.rs
+++ b/src/tools/miri/src/shims/mod.rs
@@ -11,10 +11,10 @@ mod wasi;
 mod windows;
 mod x86;
 
-pub mod ctor;
 pub mod env;
 pub mod extern_static;
 pub mod foreign_items;
+pub mod global_ctor;
 pub mod io_error;
 pub mod os_str;
 pub mod panic;
diff --git a/src/tools/miri/tests/pass/alloc-access-tracking.rs b/src/tools/miri/tests/pass/alloc-access-tracking.rs
index 0e88951dc43..9eba0ca171b 100644
--- a/src/tools/miri/tests/pass/alloc-access-tracking.rs
+++ b/src/tools/miri/tests/pass/alloc-access-tracking.rs
@@ -1,7 +1,7 @@
 #![no_std]
 #![no_main]
-//@compile-flags: -Zmiri-track-alloc-id=20 -Zmiri-track-alloc-accesses -Cpanic=abort
-//@normalize-stderr-test: "id 20" -> "id $$ALLOC"
+//@compile-flags: -Zmiri-track-alloc-id=19 -Zmiri-track-alloc-accesses -Cpanic=abort
+//@normalize-stderr-test: "id 19" -> "id $$ALLOC"
 //@only-target: linux # alloc IDs differ between OSes (due to extern static allocations)
 
 extern "Rust" {
diff --git a/src/tools/miri/tests/pass/shims/ctor.rs b/src/tools/miri/tests/pass/shims/ctor.rs
index c832e3f82a3..b997d2386b8 100644
--- a/src/tools/miri/tests/pass/shims/ctor.rs
+++ b/src/tools/miri/tests/pass/shims/ctor.rs
@@ -6,6 +6,7 @@ unsafe extern "C" fn ctor() {
     COUNT.fetch_add(1, Ordering::Relaxed);
 }
 
+#[rustfmt::skip]
 macro_rules! ctor {
     ($ident:ident = $ctor:ident) => {
         #[cfg_attr(
@@ -27,6 +28,8 @@ macro_rules! ctor {
         #[cfg_attr(windows, link_section = ".CRT$XCU")]
         #[cfg_attr(
             any(target_os = "macos", target_os = "ios"),
+            // We do not set the `mod_init_funcs` flag here since ctor/inventory also do not do
+            // that. See <https://github.com/rust-lang/miri/pull/4459#discussion_r2200115629>.
             link_section = "__DATA,__mod_init_func"
         )]
         #[used]