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

Avoid injecting sanitizer runtimes into staticlibs (#64629). #65497

Merged
merged 1 commit into from
Oct 20, 2019

Conversation

choller
Copy link
Contributor

@choller choller commented Oct 17, 2019

This fixes the remaining issue in creader.rs and also fixes the expected test failure. I have explicitly turned the $(CC) call into a negative check with the ! to ensure that this command is really failing (if it is not, then either the runtime is attached to the lib or the lib has not been instrumented and both would be an error).

I've also borrowed program.rs and the additional rustc invocation from @tmiasko 's PR since he pointed out that using -fsanitize=address with $(CC) for linking could fail if the sanitizer runtimes on the system are incompatible.

With this toolchain I was able to compile Firefox locally without any linker errors. I am still seeing races with Rust in TSan but I assume that is because I did not build with -Z build-std.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2019
@choller
Copy link
Contributor Author

choller commented Oct 17, 2019

r? @michaelwoerister

@choller
Copy link
Contributor Author

choller commented Oct 17, 2019

Also Cc @tmiasko and @nikomatsakis for additional eyes :)

@Centril
Copy link
Contributor

Centril commented Oct 17, 2019

@bors rollup=never

@nikomatsakis
Copy link
Contributor

It seems fine to me, but I don't claim a deep understanding of what's going on =)

@nikomatsakis
Copy link
Contributor

@bors delegate=tmiasko

I'm going to go ahead and give @tmiasko the ability to land this PR too, since I think they're probably the authority on this feature now. =)

@bors
Copy link
Contributor

bors commented Oct 18, 2019

✌️ @tmiasko can now approve this pull request

@nikomatsakis
Copy link
Contributor

I don't think that this implies any changes to the content in rust-lang/rustc-dev-guide#466, right?

@choller
Copy link
Contributor Author

choller commented Oct 18, 2019

This PR might actually be superseded entirely by the one from @tmiasko . The code I am patching here won't exist after their PR and I believe my idea of a negative test has been added to the other PR as well. I'm going to let @tmiasko handle the merging/closing here as appropriate.

@tmiasko
Copy link
Contributor

tmiasko commented Oct 19, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 19, 2019

📌 Commit a2feb9c has been approved by tmiasko

@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 Oct 19, 2019
@bors
Copy link
Contributor

bors commented Oct 20, 2019

⌛ Testing commit a2feb9c with merge 857a55b...

bors added a commit that referenced this pull request Oct 20, 2019
Avoid injecting sanitizer runtimes into staticlibs (#64629).

This fixes the remaining issue in `creader.rs` and also fixes the expected test failure. I have explicitly turned the `$(CC)` call into a negative check with the `!` to ensure that this command is really failing (if it is not, then either the runtime is attached to the lib or the lib has not been instrumented and both would be an error).

I've also borrowed `program.rs` and the additional `rustc` invocation from @tmiasko 's PR since he pointed out that using `-fsanitize=address` with `$(CC)` for linking could fail if the sanitizer runtimes on the system are incompatible.

With this toolchain I was able to compile Firefox locally without any linker errors. I am still seeing races with Rust in TSan but I assume that is because I did not build with `-Z build-std`.
@bors
Copy link
Contributor

bors commented Oct 20, 2019

☀️ Test successful - checks-azure
Approved by: tmiasko
Pushing 857a55b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 20, 2019
@bors bors merged commit a2feb9c into rust-lang:master Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants