Skip to content
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

Enable ASAN/LSAN/TSAN for *-apple-ios-macabi #115644

Merged
merged 2 commits into from Sep 19, 2023
Merged

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Sep 7, 2023

The -macabi targets are iOS running on MacOS, and they use the runtime libraries for MacOS, thus they have the same sanitizers available as the *-apple-darwin targets.

This is based on the work of aacf321.

Closes #113935.

@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@rust-log-analyzer

This comment has been minimized.

@danakj
Copy link
Contributor Author

danakj commented Sep 7, 2023

Along the way to writing this CL, I added the targets to the compiletest/src/util.rs file but didn't add them to the spec/ files yet. At that point trying to build for asan still failed with the unsupported message (expected) but compiletests all passed. So I am not sure that this is being tested, any thoughts?

@danakj
Copy link
Contributor Author

danakj commented Sep 7, 2023

I have verified that building Chromium with Rust in the build, targeting ios-macabi with ASAN does link and run after this change.

@danakj
Copy link
Contributor Author

danakj commented Sep 7, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

I think this may be a false positive, we're pointing to the asan libs and causing them to be built maybe instead of not when targeting *-apple-ios-macabi, but let me know if I am wrong and this does require touching the download-ci-llvm-stamp please.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprised this works at all. Shouldn't this mean the sanitizer runtimes aren't present and thus things won't load (and possibly even fail to link)? Actually, when this was tested, it was linked into chromium, so it's likely the chromium C++ build provided the sanitizer runtime.

After this is fixed, a smoke-test against pure-rust use should be done.

But in general I'm in favor of allowing this, so long as it works (once fixed and smoke tested). And like, it's an unstable feature on a tier 3 target too, so there's relatively low risk to accept.

src/bootstrap/compile.rs Outdated Show resolved Hide resolved
@tmiasko
Copy link
Contributor

tmiasko commented Sep 10, 2023

At that point trying to build for asan still failed with the unsupported message (expected) but compiletests all passed. So I am not sure that this is being tested, any thoughts?

With verbose-tests in config.toml compiletest will display a reason why tests was ignored. Sanitizers also need to be enabled in config.toml, otherwise tests with needs-sanitizer-support header will be ignored.

@danakj
Copy link
Contributor Author

danakj commented Sep 11, 2023

Trying to do a stand-alone Rust test and ./x.py test --target aarch64-apple-ios-macabi fails without any of my changes.

Building test helpers for aarch64-apple-ios-macabi
running: "xcrun" "--show-sdk-path" "--sdk" "macosx"
exit status: 0
running: "clang" "-O0" "-fPIC" "--target=arm64-apple-ios13.0-macabi" "-isysroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk" "-fembed-bitcode" "-o" "/Users/danakj/s/rust/build/aarch64-apple-ios-macabi/native/rust-test-helpers/rust_test_helpers.o" "-c" "/Users/danakj/s/rust/tests/auxiliary/rust_test_helpers.c"
cargo:warning=clang: error: invalid version number in '--target=arm64-apple-ios13.0-macabi'
exit status: 1


error occurred: Command "clang" "-O0" "-fPIC" "--target=arm64-apple-ios13.0-macabi" "-isysroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk" "-fembed-bitcode" "-o" "/Users/danakj/s/rust/build/aarch64-apple-ios-macabi/native/rust-test-helpers/rust_test_helpers.o" "-c" "/Users/danakj/s/rust/tests/auxiliary/rust_test_helpers.c" with args "clang" did not execute successfully (status code exit status: 1).

I just copied the config.example.toml and added verbose-tests=true and sanitizers=true.

However ./x.py install --target aarch64-apple-ios-macabi does work. So I can write my own test to verify the runtime is working at least.

I did:

cargo new --bin asan-test

Made a config.toml:

$ cat .cargo/config.toml
[build]
target = "aarch64-apple-ios-macabi"

[target.aarch64-apple-ios-macabi]
rustflags = [
    "-Zsanitizer=address",
    "-Clinker=clang",
    "-Clink-args=-isysroot",
    "-Clink-arg=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk"
    ]

And build and run it:

$ RUSTC=$HOME/rustbin/bin/rustc RUSTFLAGS=--sysroot=$HOME/rustbin cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/aarch64-apple-ios-macabi/debug/asan-test`
Hello, world!

@danakj
Copy link
Contributor Author

danakj commented Sep 12, 2023

Bump for review? Oh there's github UI to do this.

@danakj danakj requested a review from thomcc September 12, 2023 17:41
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Not a compiler/* reviewer so I won't r+ it.

@cjgillot
Copy link
Contributor

Thanks for the review @thomcc.
@bors r=cjgillot,thomcc

@bors
Copy link
Contributor

bors commented Sep 16, 2023

📌 Commit 1365771 has been approved by cjgillot,thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2023
@bors
Copy link
Contributor

bors commented Sep 17, 2023

⌛ Testing commit 1365771 with merge 02bd00b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2023
Enable ASAN/LSAN/TSAN for *-apple-ios-macabi

The -macabi targets are iOS running on MacOS, and they use the runtime libraries for MacOS, thus they have the same sanitizers available as the *-apple-darwin targets.

This is based on the work of rust-lang@aacf321.

Closes rust-lang#113935.
@bors
Copy link
Contributor

bors commented Sep 17, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot,thomcc
Pushing 02bd00b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 17, 2023
@bors
Copy link
Contributor

bors commented Sep 17, 2023

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (02bd00b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [3.2%, 3.5%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.505s -> 635.947s (0.07%)
Artifact size: 318.39 MiB -> 318.50 MiB (0.03%)

@danakj
Copy link
Contributor Author

danakj commented Sep 18, 2023

Thank you so much. Rust for iOS chromium is coming up. 😊

@tmiasko tmiasko removed the merged-by-bors This PR was explicitly merged by bors. label Sep 18, 2023
@tmiasko
Copy link
Contributor

tmiasko commented Sep 18, 2023

@bors retry

422 Update is not a fast forward

@bors
Copy link
Contributor

bors commented Sep 18, 2023

⌛ Testing commit 1365771 with merge cf21090...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
Enable ASAN/LSAN/TSAN for *-apple-ios-macabi

The -macabi targets are iOS running on MacOS, and they use the runtime libraries for MacOS, thus they have the same sanitizers available as the *-apple-darwin targets.

This is based on the work of rust-lang@aacf321.

Closes rust-lang#113935.
@bors
Copy link
Contributor

bors commented Sep 18, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot,thomcc
Pushing cf21090 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 18, 2023
@bors
Copy link
Contributor

bors commented Sep 18, 2023

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cf21090): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)
3.2% [2.7%, 3.4%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 633.229s -> 636.08s (0.45%)
Artifact size: 317.99 MiB -> 317.94 MiB (-0.02%)

The -macabi targets are iOS running on MacOS, and they use the runtime
libraries for MacOS, thus they have the same sanitizers available as the
*-apple-darwin targets.
Do not rename and resign the darwin sanitizers a second time for
macabi.
@danakj
Copy link
Contributor Author

danakj commented Sep 18, 2023

422 Update is not a fast forward

Hm I tried rebasing but there's no conflicts. I will upload the rebase anyhow.

@danakj
Copy link
Contributor Author

danakj commented Sep 18, 2023

@bors retry

@bors
Copy link
Contributor

bors commented Sep 18, 2023

@danakj: 🔑 Insufficient privileges: not in try users

@cjgillot
Copy link
Contributor

@bors r=cjgillot,thomcc

@bors
Copy link
Contributor

bors commented Sep 18, 2023

📌 Commit b7e98e1 has been approved by cjgillot,thomcc

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 19, 2023

⌛ Testing commit b7e98e1 with merge 19dd953...

@bors
Copy link
Contributor

bors commented Sep 19, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot,thomcc
Pushing 19dd953 to master...

@bors bors merged commit 19dd953 into rust-lang:master Sep 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (19dd953): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
-1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.4% [-1.4%, -1.4%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.066s -> 633.02s (0.31%)
Artifact size: 317.89 MiB -> 317.91 MiB (0.00%)

@danakj danakj deleted the catalyst-asan branch September 19, 2023 15:48
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 8, 2024
Pkgsrc changes:

 * Remove NetBSD-8 support (embedded LLVm requires newer C++
   than what is in -8; it's conceivable that this could still
   build with an external LLVM)
 * undo powerpc 9.0 file naming tweak, since we no longer support -8.
 * Remove patch to LLVM for powerpc now included by upstream.
 * Minor adjustments, checksum changes etc.


Upstream changes:

Version 1.74.1 (2023-12-07)
===========================

- [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM]
  (rust-lang/rust#118464)
- [Clarify guarantees for std::mem::discriminant]
  (rust-lang/rust#118006)
- [Fix some subtyping-related regressions]
  (rust-lang/rust#116415)

Version 1.74.0 (2023-11-16)
==========================

Language
--------

- [Codify that `std::mem::Discriminant<T>` does not depend on any
  lifetimes in T]
  (rust-lang/rust#104299)
- [Replace `private_in_public` lint with `private_interfaces` and
  `private_bounds` per RFC 2145]
  (rust-lang/rust#113126)
  Read more in
  [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html).
- [Allow explicit `#[repr(Rust)]`]
  (rust-lang/rust#114201)
- [closure field capturing: don't depend on alignment of packed fields]
  (rust-lang/rust#115315)
- [Enable MIR-based drop-tracking for `async` blocks]
  (rust-lang/rust#107421)

Compiler
--------

- [stabilize combining +bundle and +whole-archive link modifiers]
  (rust-lang/rust#113301)
- [Stabilize `PATH` option for `--print KIND=PATH`]
  (rust-lang/rust#114183)
- [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`]
  (rust-lang/rust#115644)
- [Promote loongarch64-unknown-none* to Tier 2]
  (rust-lang/rust#115368)
- [Add `i686-pc-windows-gnullvm` as a tier 3 target]
  (rust-lang/rust#115687)

Libraries
---------

- [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err]
  (rust-lang/rust#98704)
- [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`]
  (rust-lang/rust#111278)
- [impl Step for IP addresses]
  (rust-lang/rust#113748)
- [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`]
  (rust-lang/rust#114041)
- [`impl TryFrom<char> for u16`]
  (rust-lang/rust#114065)
- [Stabilize `io_error_other` feature]
  (rust-lang/rust#115453)
- [Stabilize the `Saturating` type]
  (rust-lang/rust#115477)
- [Stabilize const_transmute_copy]
  (rust-lang/rust#115520)

Stabilized APIs
---------------

- [`core::num::Saturating`]
  (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html)
- [`impl From<io::Stdout> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`std::ffi::OsString::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsString::into_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes)
- [`std::ffi::OsStr::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsStr::as_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes)
- [`std::io::Error::other`]
  (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other)
- [`impl TryFrom<char> for u16`]
  (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16)
- [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

These APIs are now stable in const contexts:

- [`core::mem::transmute_copy`]
  (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html)
- [`str::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii)
- [`[u8]::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii)

Cargo
-----

- [fix: Set MSRV for internal packages]
  (rust-lang/cargo#12381)
- [config: merge lists in precedence order]
  (rust-lang/cargo#12515)
- [fix(update): Clarify meaning of --aggressive as --recursive]
  (rust-lang/cargo#12544)
- [fix(update): Make `-p` more convenient by being positional]
  (rust-lang/cargo#12545)
- [feat(help): Add styling to help output ]
  (rust-lang/cargo#12578)
- [feat(pkgid): Allow incomplete versions when unambigious]
  (rust-lang/cargo#12614)
- [feat: stabilize credential-process and registry-auth]
  (rust-lang/cargo#12649)
- [feat(cli): Add '-n' to dry-run]
  (rust-lang/cargo#12660)
- [Add support for `target.'cfg(..)'.linker`]
  (rust-lang/cargo#12535)
- [Stabilize `--keep-going`]
  (rust-lang/cargo#12568)
- [feat: Stabilize lints]
  (rust-lang/cargo#12648)

Rustdoc
-------

- [Add warning block support in rustdoc]
  (rust-lang/rust#106561)
- [Accept additional user-defined syntax classes in fenced code blocks]
  (rust-lang/rust#110800)
- [rustdoc-search: add support for type parameters]
  (rust-lang/rust#112725)
- [rustdoc: show inner enum and struct in type definition for concrete type]
  (rust-lang/rust#114855)

Compatibility Notes
-------------------

- [Raise minimum supported Apple OS versions]
  (rust-lang/rust#104385)
- [make Cell::swap panic if the Cells partially overlap]
  (rust-lang/rust#114795)
- [Reject invalid crate names in `--extern`]
  (rust-lang/rust#116001)
- [Don't resolve generic impls that may be shadowed by dyn built-in impls]
  (rust-lang/rust#114941)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

None this cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

Rust has no ASAN support for catalyst ABI (x86_64-apple-ios-macabi)
8 participants