Uh oh!
There was an error while loading. Please reload this page.
Enable ASan, TSan, UBSan for aarch64-apple-darwin. - #79883
Conversation
rust-highfive
commented
Dec 10, 2020
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
frewsxcv
commented
Dec 10, 2020
I'm compiling Rust locally with these changes to confirm. |
frewsxcv
commented
Dec 10, 2020
I will investigate tomorrow |
lcnr
commented
Dec 10, 2020
can't really review this, as you've already requested a review by @shepmaster r? @shepmaster |
frewsxcv
commented
Dec 10, 2020
Regarding my last comment, I forgot to enable These warning occur when we run So it seems that by changing the dynamic libraries' names, we invalidate the signature. If I manually re-sign via: Then ASAN seems to work: |
alex
commented
Dec 10, 2020
That's not really the ASAN error I'd expect from that code, FWIW -- what I get on x86 looks way more sensible. |
frewsxcv
commented
Dec 10, 2020
I was just wondering about this. It seems like ASan is failing to allocate memory for itself and this is just the error log? Hm |
frewsxcv
commented
Dec 10, 2020
Here's the LLDB output, which maybe confirms that: |
alex
commented
Dec 10, 2020
I agree that it looks like ASAN is failing to set itself up. |
@saleemrashid pointed me to llvm/llvm-project@176a6e7 which I cherry-picked on top of Rust's current LLVM commit and it fixes the previous issue! d517333 |
frewsxcv
commented
Dec 11, 2020
This is ready for review! Note that this pull request is blocked on rust-lang/llvm-project#86 |
frewsxcv
commented
Dec 18, 2020
rust-lang/llvm-project#86 has merged so it's now just waiting for a review |
bors
commented
Jan 1, 2021
☔ The latest upstream changes (presumably #80566) made this pull request unmergeable. Please resolve the merge conflicts. |
frewsxcv
commented
Jan 1, 2021
Merge conflicts resolved |
alex
left a comment
There was a problem hiding this comment.
One question, otherwise looks good to me (with the caveat that I'm far from a rustc expert!)
| "x86_64-unknown-linux-gnu", | ||
| ]; | ||
| const MSAN_SUPPORTED_TARGETS: &[&str] = | ||
| &["aarch64-unknown-linux-gnu", "x86_64-unknown-freebsd", "x86_64-unknown-linux-gnu"]; |
There was a problem hiding this comment.
I returned my new Macbook Air (need more memory and waiting for the new one to ship), so I can't easily confirm, but I recall there being an error message saying macOS is not supported by MSan, which is maybe why x86_64-apple-darwin is also not listed here. These docs also back up this claim
There was a problem hiding this comment.
I clearly have the reading comprehension of a goldfish, I totally missed that no macOS platforms were there.
There was a problem hiding this comment.
if (COMPILER_RT_HAS_SANITIZER_COMMON AND MSAN_SUPPORTED_ARCH AND
OS_NAME MATCHES"Linux|FreeBSD|NetBSD")
set(COMPILER_RT_HAS_MSAN TRUE)
else()
set(COMPILER_RT_HAS_MSAN FALSE)
endif()| // Upon renaming the install name, the code signature of the file will invalidate, | ||
| // so we will sign it again. |
There was a problem hiding this comment.
I don't follow this. Renaming other signed files doesn't cause problems; why does this?
There was a problem hiding this comment.
Renaming the file with mv doesn't invalidate the signature, but renaming the install name (within the contents of the file) with install_name_tool does invalidate.
Searching for "install_name_tool invalidate signature" on Google returns other projects that have hit this. For example: https://community.intel.com/t5/Intel-oneAPI-Threading-Building/Mac-install-location-conflicts-with-code-signing/m-p/1008483/highlight/true#M12505
shepmaster
commented
Jan 1, 2021
@bors r+ rollup My understanding is that these tools aren't critical so if we realize that this breaks them somehow, it won't be a big thing until we can fix them. |
bors
commented
Jan 1, 2021
📌 Commit d482de3 has been approved by |
frewsxcv
commented
Jan 1, 2021
Thanks for the review! What you said sounds right. The only functionality I'm changing for existing platforms is the addition of that extra code signing step for macOS machines. So if we saw an regressions on other platforms, I'd expect it to be just limited to macOS, and that code addition in particular. |
bors
commented
Jan 2, 2021
bors
commented
Jan 2, 2021
☀️ Test successful - checks-actions |
I confirmed ASan, TSan, UBSan all work for me locally with
clangon my new Macbook Air.This requires rust-lang/llvm-project#86