chore: Allow Rust ASAN flags to propagate into v8 build (#1449)

.github/workflows/ci.yml

@@ -35,6 +35,10 @@ jobs:
             target: x86_64-apple-darwin
             variant: release
 
+          - os: macos-14
+            target: aarch64-apple-darwin
+            variant: asan
+
           - os: macos-14
             target: aarch64-apple-darwin
             variant: debug
@@ -128,25 +132,6 @@ jobs:
           restore-keys:
             cargo-${{ matrix.config.target }}-${{ matrix.config.variant }}-
 
-        # It seems that the 'target' directory does not always get restored
-        # from cache correctly on MacOS. In the build log we see the following:
-        #
-        #   Fresh serde_derive v1.0.115
-        #
-        # But a little while after that Cargo aborts because 'serde_derive' is
-        # now nowhere to be found. We're not the only ones experiencing this,
-        # see https://github.com/actions-rs/cargo/issues/111.
-        #
-        #   error[E0463]: can't find crate for `serde_derive`
-        #   ##[error]   --> /Users/runner/.cargo/registry/src/github.com-
-        #       |           1ecc6299db9ec823/serde-1.0.115/src/lib.rs:285:1
-        #       |
-        #   285 | extern crate serde_derive;
-        #       | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate
-      - name: Work around MacOS + Cargo + Github Actions cache bug
-        if: runner.os == 'macOS'
-        run: cargo clean -p serde_derive
-
       - name: Install and start sccache
         shell: pwsh
         env:
@@ -169,9 +154,26 @@ jobs:
           . $basename/sccache --start-server
           echo "$(pwd)/$basename" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
 
+      - name: Install Rust (nightly)
+        uses: dtolnay/rust-toolchain@nightly
+        if: matrix.config.variant == 'asan'
+
+      - name: Test (ASAN)
+        env:
+          SCCACHE_IDLE_TIMEOUT: 0
+        if: matrix.config.variant == 'asan'
+        # https://github.com/rust-lang/rust/issues/87215
+        run: |
+          rustup component add rust-src --toolchain nightly-${{ matrix.config.target }}
+          # Is there a better way to install this tool?
+          curl -O http://commondatastorage.googleapis.com/chromium-browser-clang-staging/Mac/dsymutil-llvmorg-17-init-19076-g5533fc10-1.tgz
+          tar -C tools/clang/dsymutil/ -xvzf dsymutil-llvmorg-17-init-19076-g5533fc10-1.tgz
+          V8_FROM_SOURCE=true RUSTFLAGS="-C opt-level=1 -Zsanitizer=address" cargo +nightly -Z build-std test --lib --bins --tests -vv --target ${{ matrix.config.target }}
+
       - name: Test
         env:
           SCCACHE_IDLE_TIMEOUT: 0
+        if: matrix.config.variant == 'debug' || matrix.config.variant == 'release'
         run:
           cargo test -vv --all-targets --locked ${{ env.CARGO_VARIANT_FLAG }}
           --target ${{ matrix.config.target }}
@@ -185,6 +187,7 @@ jobs:
         run: cargo fmt -- --check
 
       - name: Prepare binary publish
+        if: matrix.config.variant == 'debug' || matrix.config.variant == 'release'
         run: gzip -9c
           target/${{ matrix.config.target }}/${{ matrix.config.variant }}/gn_out/obj/${{ env.LIB_NAME }}.${{ env.LIB_EXT }} >
           target/${{ env.LIB_NAME }}_${{ matrix.config.variant }}_${{ matrix.config.target }}.${{ env.LIB_EXT }}.gz &&
@@ -194,7 +197,8 @@ jobs:
         uses: softprops/action-gh-release@v0.1.15
         if: >-
           github.repository == 'denoland/rusty_v8' &&
-          startsWith(github.ref, 'refs/tags/')
+          startsWith(github.ref, 'refs/tags/') &&
+          (matrix.config.variant == 'debug' || matrix.config.variant == 'release')
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         with:

Cargo.lock

@@ -1103,6 +1103,12 @@ dependencies = [
  "windows-sys 0.52.0",
 ]
 
+[[package]]
+name = "rustversion"
+version = "1.0.15"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "80af6f9131f277a45a3fba6ce8e2258037bb0477a67e610d3c1fe046ab31de47"
+
 [[package]]
 name = "ryu"
 version = "1.0.17"
@@ -1327,6 +1333,7 @@ dependencies = [
  "home",
  "miniz_oxide",
  "once_cell",
+ "rustversion",
  "trybuild",
  "which",
 ]

Cargo.toml

@@ -75,6 +75,13 @@ exclude = [
   "!v8/tools/testrunner/utils/dump_build_config.py",
 ]
 
+[profile.dev]
+# rusty_v8 may miscompile at opt-level=0.
+# https://github.com/rust-lang/rust/issues/87215
+# https://github.com/rust-lang/rust/issues/75839
+# https://github.com/rust-lang/rust/issues/121028
+opt-level = 1
+
 [features]
 default = ["use_custom_libcxx"]
 use_custom_libcxx = []
@@ -99,6 +106,7 @@ fslock = "0.2"
 trybuild = "1.0.61"
 which = "5"
 home = "0"
+rustversion = "1"
 
 [[example]]
 name = "hello_world"

build.rs

@@ -49,6 +49,7 @@ fn main() {
     "DISABLE_CLANG",
     "EXTRA_GN_ARGS",
     "NO_PRINT_GN_ARGS",
+    "CARGO_ENCODED_RUSTFLAGS",
   ];
   for env in envs {
     println!("cargo:rerun-if-env-changed={}", env);
@@ -81,9 +82,22 @@ fn main() {
     return;
   }
 
+  let is_asan = if let Some(rustflags) = env::var_os("CARGO_ENCODED_RUSTFLAGS")
+  {
+    if std::env::var_os("OPT_LEVEL").unwrap_or_default() == "0" {
+      panic!("v8 crate cannot be compiled with OPT_LEVEL=0 and ASAN.\nTry `[profile.dev.package.v8] opt-level = 1`.\nAborting before miscompilations cause issues.");
+    }
+
+    let rustflags = rustflags.to_string_lossy();
+    rustflags.find("-Z sanitizer=address").is_some()
+      || rustflags.find("-Zsanitizer=address").is_some()
+  } else {
+    false
+  };
+
   // Build from source
   if env::var_os("V8_FROM_SOURCE").is_some() {
-    return build_v8();
+    return build_v8(is_asan);
   }
 
   // utilize a lockfile to prevent linking of
@@ -105,7 +119,7 @@ fn main() {
   lockfile.unlock().expect("Couldn't unlock lockfile");
 }
 
-fn build_v8() {
+fn build_v8(is_asan: bool) {
   env::set_var("DEPOT_TOOLS_WIN_TOOLCHAIN", "0");
 
   // cargo publish doesn't like pyc files.
@@ -132,6 +146,10 @@ fn build_v8() {
     vec!["is_debug=false".to_string()]
   };
 
+  if is_asan {
+    gn_args.push("is_asan=true".to_string());
+  }
+
   if cfg!(not(feature = "use_custom_libcxx")) {
     gn_args.push("use_custom_libcxx=false".to_string());
   }
@@ -146,11 +164,11 @@ fn build_v8() {
     // -gline-tables-only is Clang-only
     gn_args.push("line_tables_only=false".into());
   } else if let Some(clang_base_path) = find_compatible_system_clang() {
-    println!("clang_base_path {}", clang_base_path.display());
+    println!("clang_base_path (system): {}", clang_base_path.display());
     gn_args.push(format!("clang_base_path={:?}", clang_base_path));
     gn_args.push("treat_warnings_as_errors=false".to_string());
   } else {
-    println!("using Chromiums clang");
+    println!("using Chromium's clang");
     let clang_base_path = clang_download();
     gn_args.push(format!("clang_base_path={:?}", clang_base_path));
 
@@ -661,7 +679,7 @@ fn find_compatible_system_clang() -> Option<PathBuf> {
 // modify the source directory.
 fn clang_download() -> PathBuf {
   let clang_base_path = build_dir().join("clang");
-  println!("clang_base_path {}", clang_base_path.display());
+  println!("clang_base_path (downloaded) {}", clang_base_path.display());
   assert!(Command::new(python())
     .arg("./tools/clang/scripts/update.py")
     .arg("--output-dir")
@@ -826,7 +844,7 @@ pub fn maybe_gen(manifest_dir: &str, gn_args: GnArgs) -> PathBuf {
       .stderr(Stdio::inherit())
       .envs(env::vars())
       .status()
-      .expect("Coud not run `gn`")
+      .expect("Could not run `gn`")
       .success());
   }
   gn_out_dir
@@ -973,11 +991,4 @@ edge [fontsize=10]
     assert!(files.contains("../../../example/src/count_bytes.py"));
     assert!(!files.contains("obj/hello/hello.o"));
   }
-
-  #[test]
-  fn test_static_lib_size() {
-    let static_lib_size = std::fs::metadata(static_lib_path()).unwrap().len();
-    eprintln!("static lib size {}", static_lib_size);
-    assert!(static_lib_size <= 300u64 << 20); // No more than 300 MiB.
-  }
 }

src/V8.rs

@@ -67,7 +67,7 @@ enum GlobalState {
   Uninitialized,
   PlatformInitialized(SharedRef<Platform>),
   Initialized(SharedRef<Platform>),
-  Disposed(SharedRef<Platform>),
+  Disposed(#[allow(dead_code)] SharedRef<Platform>),
   PlatformShutdown,
 }
 use GlobalState::*;

src/binding.cc

@@ -2136,19 +2136,27 @@ const v8::FunctionTemplate* v8__FunctionTemplate__New(
     v8::SideEffectType side_effect_type, void* func_ptr1,
     const v8::CFunctionInfo* c_function_info1, void* func_ptr2,
     const v8::CFunctionInfo* c_function_info2) {
-  auto overload = v8::MemorySpan<const v8::CFunction>{};
   // Support upto 2 overloads. V8 requires TypedArray to have a
   // v8::Array overload.
   if (func_ptr1) {
     if (func_ptr2 == nullptr) {
       const v8::CFunction o[] = {v8::CFunction(func_ptr1, c_function_info1)};
-      overload = v8::MemorySpan<const v8::CFunction>{o, 1};
+      auto overload = v8::MemorySpan<const v8::CFunction>{o, 1};
+      return local_to_ptr(v8::FunctionTemplate::NewWithCFunctionOverloads(
+          isolate, callback, ptr_to_local(data_or_null),
+          ptr_to_local(signature_or_null), length, constructor_behavior,
+          side_effect_type, overload));
     } else {
       const v8::CFunction o[] = {v8::CFunction(func_ptr1, c_function_info1),
                                  v8::CFunction(func_ptr2, c_function_info2)};
-      overload = v8::MemorySpan<const v8::CFunction>{o, 2};
+      auto overload = v8::MemorySpan<const v8::CFunction>{o, 2};
+      return local_to_ptr(v8::FunctionTemplate::NewWithCFunctionOverloads(
+          isolate, callback, ptr_to_local(data_or_null),
+          ptr_to_local(signature_or_null), length, constructor_behavior,
+          side_effect_type, overload));
     }
   }
+  auto overload = v8::MemorySpan<const v8::CFunction>{};
   return local_to_ptr(v8::FunctionTemplate::NewWithCFunctionOverloads(
       isolate, callback, ptr_to_local(data_or_null),
       ptr_to_local(signature_or_null), length, constructor_behavior,

src/isolate.rs

@@ -54,6 +54,7 @@ use std::mem::MaybeUninit;
 use std::ops::Deref;
 use std::ops::DerefMut;
 use std::ptr;
+use std::ptr::addr_of_mut;
 use std::ptr::drop_in_place;
 use std::ptr::null_mut;
 use std::ptr::NonNull;
@@ -509,7 +510,7 @@ extern "C" {
 
   fn v8__HeapProfiler__TakeHeapSnapshot(
     isolate: *mut Isolate,
-    callback: extern "C" fn(*mut c_void, *const u8, usize) -> bool,
+    callback: unsafe extern "C" fn(*mut c_void, *const u8, usize) -> bool,
     arg: *mut c_void,
   );
 
@@ -1279,7 +1280,7 @@ impl Isolate {
   where
     F: FnMut(&[u8]) -> bool,
   {
-    extern "C" fn trampoline<F>(
+    unsafe extern "C" fn trampoline<F>(
       arg: *mut c_void,
       data: *const u8,
       size: usize,
@@ -1287,14 +1288,18 @@ impl Isolate {
     where
       F: FnMut(&[u8]) -> bool,
     {
-      let p = arg as *mut F;
-      let callback = unsafe { &mut *p };
-      let slice = unsafe { std::slice::from_raw_parts(data, size) };
-      callback(slice)
+      let mut callback = NonNull::<F>::new_unchecked(arg as _);
+      if size > 0 {
+        (callback.as_mut())(std::slice::from_raw_parts(data, size))
+      } else {
+        (callback.as_mut())(&[])
+      }
     }
 
-    let arg = &mut callback as *mut F as *mut c_void;
-    unsafe { v8__HeapProfiler__TakeHeapSnapshot(self, trampoline::<F>, arg) }
+    let arg = addr_of_mut!(callback);
+    unsafe {
+      v8__HeapProfiler__TakeHeapSnapshot(self, trampoline::<F>, arg as _)
+    }
   }
 
   /// Set the default context to be included in the snapshot blob.

src/scope.rs

@@ -2220,7 +2220,7 @@ mod tests {
   /// assigning a value to a variable with an explicitly stated type is that the
   /// latter allows coercions and dereferencing to change the type, whereas
   /// `AssertTypeOf` requires the compared types to match exactly.
-  struct AssertTypeOf<'a, T>(pub &'a T);
+  struct AssertTypeOf<'a, T>(#[allow(dead_code)] &'a T);
   impl<'a, T> AssertTypeOf<'a, T> {
     pub fn is<A>(self)
     where

tests/test_api.rs

@@ -11,7 +11,7 @@ use std::ffi::CStr;
 use std::hash::Hash;
 use std::mem::MaybeUninit;
 use std::os::raw::c_char;
-use std::ptr::{addr_of, NonNull};
+use std::ptr::{addr_of, addr_of_mut, NonNull};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::Arc;
 use std::sync::Mutex;
@@ -8381,7 +8381,7 @@ fn clear_kept_objects() {
 #[test]
 fn wasm_streaming_callback() {
   thread_local! {
-    static WS: RefCell<Option<v8::WasmStreaming>> = RefCell::new(None);
+    static WS: RefCell<Option<v8::WasmStreaming>> = const { RefCell::new(None) };
   }
 
   let callback = |scope: &mut v8::HandleScope,
@@ -8593,7 +8593,7 @@ fn oom_callback() {
 #[test]
 fn prepare_stack_trace_callback() {
   thread_local! {
-    static SITES: RefCell<Option<v8::Global<v8::Array>>> = RefCell::new(None);
+    static SITES: RefCell<Option<v8::Global<v8::Array>>> = const { RefCell::new(None) };
   }
 
   let script = r#"
@@ -8692,7 +8692,9 @@ fn icu_date() {
 
 #[test]
 fn icu_set_common_data_fail() {
-  assert!(v8::icu::set_common_data_73(&[1, 2, 3]).is_err());
+  assert!(
+    v8::icu::set_common_data_73(&[1, 2, 3, 0, 0, 0, 0, 0, 0, 0, 0]).is_err()
+  );
 }
 
 #[test]
@@ -10578,7 +10580,7 @@ fn test_fast_calls_callback_options_data() {
 
   let global = context.global(scope);
   let external =
-    v8::External::new(scope, unsafe { &mut DATA as *mut bool as *mut c_void });
+    v8::External::new(scope, unsafe { addr_of_mut!(DATA) as *mut c_void });
 
   let template = v8::FunctionTemplate::builder(slow_fn)
     .data(external.into())

tests/test_ui.rs

@@ -1,6 +1,6 @@
-// Don't run UI tests on emulated environment.
+// Don't run UI tests on emulated environment or nightly build.
 #[cfg(not(target_os = "android"))]
-#[test]
+#[rustversion::attr(not(nightly), test)]
 fn ui() {
   // This environment variable tells build.rs that we're running trybuild tests,
   // so it won't rebuild V8.

tools/clang

@@ -1 +1 @@
-Subproject commit 3344dd8997f422862a1c5477b490b3611be31351
+Subproject commit 5016a8b16c6e3c34138e776dce423fec7b6cf610