From 0506784a2a4cf3aa44ca2676728714d03a4aa618 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 7 Dec 2023 04:17:39 +0100 Subject: [PATCH 1/2] Add Main Thread Checker --- README.md | 8 ++- src/main.rs | 85 +++++++++++++++++++++-- test/ci.sh | 17 +++++ test/test-main_thread_checker/Cargo.lock | 32 +++++++++ test/test-main_thread_checker/Cargo.toml | 8 +++ test/test-main_thread_checker/src/main.rs | 18 +++++ 6 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 test/test-main_thread_checker/Cargo.lock create mode 100644 test/test-main_thread_checker/Cargo.toml create mode 100644 test/test-main_thread_checker/src/main.rs diff --git a/README.md b/README.md index 3440eae..c29b623 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ itself. ### Sanitizing `cargo careful` can additionally build and run your program and standard library -with a sanitizer. This feature is experimental and disabled by default. +with a sanitizer. This feature is experimental and disabled by default. The [underlying `rustc` feature](https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/sanitizer.html) doesn't play well with [procedural macros](https://doc.rust-lang.org/reference/procedural-macros.html). @@ -86,6 +86,12 @@ setting `ASAN_OPTIONS=detect_leaks=0` in your program's environment, as memory l usually a soundness or correctness issue. If you set the `ASAN_OPTIONS` environment variable yourself (to any value, including an empty string), that will override this behavior. +### Main Thread Checker + +`cargo careful` automatically enables [Apple's Main Thread Checker](https://developer.apple.com/documentation/xcode/diagnosing-memory-thread-and-crash-issues-early#Detect-improper-UI-updates-on-background-threads) on macOS, iOS, tvOS and watchOS targets, whenever the user has Xcode installed. + +This helps diagnosing issues with executing thread-unsafe functionality off the main thread on those platforms. + ### `cfg` flag `cargo careful` sets the `careful` configuration flag, so you can use Rust's compile-time diff --git a/src/main.rs b/src/main.rs index 399705b..1e801d4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,9 +3,9 @@ use std::env; use std::ffi::OsString; use std::path::PathBuf; -use std::process::{self, Command}; +use std::process::{self, Command, Stdio}; -use anyhow::{bail, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use rustc_build_sysroot::{BuildMode, SysrootBuilder, SysrootConfig}; use rustc_version::VersionMeta; @@ -38,6 +38,48 @@ pub fn rustc_version_info() -> VersionMeta { VersionMeta::for_command(rustc()).expect("failed to determine rustc version") } +/// Find the path for Apple's Main Thread Checker on the current system. +/// +/// This is intended to be used on macOS, but should work on other systems +/// that have something similar to XCode set up. +fn main_thread_checker_path() -> Result> { + // Find the Xcode developer directory, usually one of: + // - /Applications/Xcode.app/Contents/Developer + // - /Library/Developer/CommandLineTools + // + // This could be done by the `apple-sdk` crate, but we avoid the dependency here. + let output = Command::new("xcode-select") + .args(["--print-path"]) + .stderr(Stdio::null()) + .output() + .context("`xcode-select --print-path` failed to run")?; + + if !output.status.success() { + return Err(anyhow!( + "got error when running `xcode-select --print-path`:\n{:?}", + output, + )); + } + + let stdout = String::from_utf8(output.stdout) + .context("`xcode-select --print-path` returned invalid UTF-8")?; + let developer_dir = PathBuf::from(stdout.trim()); + + // Introduced in XCode 9.0, and has not changed location since. + // + let path = developer_dir.join("usr/lib/libMainThreadChecker.dylib"); + if path.try_exists()? { + Ok(Some(path)) + } else { + eprintln!( + "warn: libMainThreadChecker.dylib could not be found at {}", + path.display() + ); + eprintln!(" This usually means you're using the Xcode command line tools, which does not have this capability."); + Ok(None) + } +} + // Computes the extra flags that need to be passed to cargo to make it behave like the current // cargo invocation. fn cargo_extra_flags() -> Vec { @@ -235,7 +277,7 @@ fn build_sysroot( sysroot_dir } -fn cargo_careful(args: env::Args) { +fn cargo_careful(args: env::Args) -> Result<()> { let mut args = args.peekable(); let rustc_version = rustc_version_info(); @@ -285,6 +327,7 @@ fn cargo_careful(args: env::Args) { // Forward regular argument. cargo_args.push(arg); } + // The rest is for cargo to forward to the binary / test runner. cargo_args.push("--".into()); cargo_args.extend(args); @@ -317,7 +360,7 @@ fn cargo_careful(args: env::Args) { Some(c) => c, None => { // We just did the setup. - return; + return Ok(()); } }; @@ -339,6 +382,34 @@ fn cargo_careful(args: env::Args) { cmd.args(["--target", target.as_str()]); } + // Enable Main Thread Checker on macOS targets, as documented here: + // + // + // On iOS, tvOS and watchOS simulators, the path is somewhere inside the + // simulator runtime, which is more difficult to find, so we don't do that + // yet (those target also probably wouldn't run in `cargo-careful`, so ). + // + // Note: The main thread checker by default removes itself from + // `DYLD_INSERT_LIBRARIES` upon load, see `MTC_RESET_INSERT_LIBRARIES`: + // + // This means that it is not inherited by child processes, so we have to + // tell Cargo to set this environment variable for the processes it + // launches (instead of just setting it for Cargo itself using `cmd.env`). + // + // Note: We do this even if the host is not running macOS, even though the + // environment variable will also be passed to any rustc processes that + // Cargo spawns (as Cargo doesn't currently have a good way of only + // specifying environment variables to only the binary being run). + // This is probably fine though, the environment variable is + // Apple-specific and likely just be ignored on other hosts. + if target.contains("-darwin") { + if let Some(path) = main_thread_checker_path()? { + cmd.arg("--config"); + // TODO: Quote the path correctly according to toml rules + cmd.arg(format!("env.DYLD_INSERT_LIBRARIES={path:?}")); + } + } + cmd.args(cargo_args); // Setup environment. Both rustc and rustdoc need these flags. @@ -357,10 +428,10 @@ fn cargo_careful(args: env::Args) { } // Run it! - exec(cmd, (verbose > 0).then_some("[cargo-careful] ")); + exec(cmd, (verbose > 0).then_some("[cargo-careful] ")) } -fn main() { +fn main() -> Result<()> { let mut args = env::args(); // Skip binary name. args.next().unwrap(); @@ -373,7 +444,7 @@ fn main() { match first.as_str() { "careful" => { // It's us! - cargo_careful(args); + cargo_careful(args) } _ => { show_error!( diff --git a/test/ci.sh b/test/ci.sh index 2490e22..cd0d67b 100755 --- a/test/ci.sh +++ b/test/ci.sh @@ -18,3 +18,20 @@ cargo careful setup --target x86_64-unknown-none cargo careful build --target x86_64-unknown-none --locked cargo clean popd + +# test Apple's Main Thread Checker +if uname -s | grep -q "Darwin" +then + pushd test-main_thread_checker + # Run as normal; this will output warnings, but not fail + cargo careful run --locked + # Run with flag that tells the Main Thread Checker to fail + # See + if MTC_CRASH_ON_REPORT=1 cargo careful run --locked + then + echo "Main Thread Checker did not crash" + exit 1 + fi + cargo clean + popd +fi diff --git a/test/test-main_thread_checker/Cargo.lock b/test/test-main_thread_checker/Cargo.lock new file mode 100644 index 0000000..17b3392 --- /dev/null +++ b/test/test-main_thread_checker/Cargo.lock @@ -0,0 +1,32 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "objc-sys" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c7c71324e4180d0899963fc83d9d241ac39e699609fc1025a850aadac8257459" + +[[package]] +name = "objc2" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a9c7f0d511a4ce26b078183179dca908171cfc69f88986fe36c5138e1834476" +dependencies = [ + "objc-sys", + "objc2-encode", +] + +[[package]] +name = "objc2-encode" +version = "4.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2ff06a6505cde0766484f38d8479ac8e6d31c66fbc2d5492f65ca8c091456379" + +[[package]] +name = "test-cargo-careful-main_thread_checker" +version = "0.1.0" +dependencies = [ + "objc2", +] diff --git a/test/test-main_thread_checker/Cargo.toml b/test/test-main_thread_checker/Cargo.toml new file mode 100644 index 0000000..d7c88d9 --- /dev/null +++ b/test/test-main_thread_checker/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "test-cargo-careful-main_thread_checker" +version = "0.1.0" +edition = "2021" +publish = false + +[target.'cfg(target_os = "macos")'.dependencies] +objc2 = "0.5.0" diff --git a/test/test-main_thread_checker/src/main.rs b/test/test-main_thread_checker/src/main.rs new file mode 100644 index 0000000..b9922a6 --- /dev/null +++ b/test/test-main_thread_checker/src/main.rs @@ -0,0 +1,18 @@ +//! Run [NSView new] on a separate thread, which should get caught by the +//! main thread checker. +use objc2::rc::Id; +use objc2::runtime::AnyObject; +use objc2::{class, msg_send_id}; + +#[link(name = "AppKit", kind = "framework")] +extern "C" {} + +fn main() { + std::thread::scope(|s| { + s.spawn(|| { + // Note: Usually you'd use `icrate::NSView::new`, this is to + // avoid the heavy dependency. + let _: Id = unsafe { msg_send_id![class!(NSView), new] }; + }); + }); +} From 88f11cbfc71eec4b34bedf6bf3647e667b2179da Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 15 Jan 2024 07:02:19 +0000 Subject: [PATCH 2/2] tweak comment --- src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1e801d4..0b61b15 100644 --- a/src/main.rs +++ b/src/main.rs @@ -387,7 +387,7 @@ fn cargo_careful(args: env::Args) -> Result<()> { // // On iOS, tvOS and watchOS simulators, the path is somewhere inside the // simulator runtime, which is more difficult to find, so we don't do that - // yet (those target also probably wouldn't run in `cargo-careful`, so ). + // yet (those target also probably wouldn't run in `cargo-careful` anyway). // // Note: The main thread checker by default removes itself from // `DYLD_INSERT_LIBRARIES` upon load, see `MTC_RESET_INSERT_LIBRARIES`: @@ -401,7 +401,7 @@ fn cargo_careful(args: env::Args) -> Result<()> { // Cargo spawns (as Cargo doesn't currently have a good way of only // specifying environment variables to only the binary being run). // This is probably fine though, the environment variable is - // Apple-specific and likely just be ignored on other hosts. + // Apple-specific and will likely be ignored on other hosts. if target.contains("-darwin") { if let Some(path) = main_thread_checker_path()? { cmd.arg("--config");