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.

Sign up for GitHub

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

Jump to bottom

Diagnose use of incompatible sanitizers #73347

Merged
merged 1 commit into from
Jun 20, 2020
Merged

Diagnose use of incompatible sanitizers #73347

merged 1 commit into from
Jun 20, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jun 14, 2020

Emit an error when incompatible sanitizer are configured through command
line options. Previously the last one configured prevailed and others
were silently ignored.

Additionally use a set to represent configured sanitizers, making it
possible to enable multiple sanitizers at once. At least in principle,
since currently all of them are considered to be incompatible with
others.

@tmiasko
Diagnose use of incompatible sanitizers
0a65f28 
Emit an error when incompatible sanitizer are configured through command
line options. Previously the last one configured prevailed and others
were silently ignored.

Additionally use a set to represent configured sanitizers, making it
possible to enable multiple sanitizers at once. At least in principle,
since currently all of them are considered to be incompatible with
others.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2020
@petrochenkov
Copy link
Contributor

@rustbot ping llvm
Could someone else review this? I'm overloaded.

@rustbot rustbot added the ICEBreaker-LLVM Bugs identified for the LLVM ICE-breaker group label Jun 14, 2020
@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2020

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @camelid @comex @cuviper @DutchGhost @dyncat @hanna-kruppe @hdhoang @heyrutvik @JOE1994 @jryans @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @spastorino @vertexclique

@petrochenkov
Copy link
Contributor

Omg, so many pings.
I expected to see WG-LLVM.
r? @cuviper

@nikic
Copy link
Contributor

nikic commented Jun 14, 2020

Looks very nice :) I presume the next step is to remove the conflict from address+leak?

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2020

📌 Commit 0a65f28 has been approved by nikic

@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 Jun 14, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 14, 2020

In clang -fsanitize=address,leak is equivalent to -fsanitize=address. Using
both options doesn't change whether LeakSanitizer is enabled by default, and on
macOS it will remain disabled unless combined with ASAN_OPTIONS=detect_leaks=1.

In rustc we aren't in position to improve the situation so we would end up with
similar behaviour. In general my opinion about allowing both is ¯\_(ツ)_/¯.

@Manishearth Manishearth mentioned this pull request Jun 20, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
@bors
Auto merge of rust-lang#73528 - Manishearth:rollup-7djz8nd, r=Manishe…
033013c 
…arth

Rollup of 16 pull requests

Successful merges:

 - rust-lang#71420 (Specialization is unsound)
 - rust-lang#71899 (Refactor `try_find` a little)
 - rust-lang#72689 (add str to common types)
 - rust-lang#72791 (update coerce docs and unify relevant tests)
 - rust-lang#72934 (forbid mutable references in all constant contexts except for const-fns)
 - rust-lang#73027 (Make `need_type_info_err` more conservative)
 - rust-lang#73347 (Diagnose use of incompatible sanitizers)
 - rust-lang#73359 (shim.rs: avoid creating `Call` terminators calling `Self`)
 - rust-lang#73399 (Clean up E0668 explanation)
 - rust-lang#73436 (Clean up E0670 explanation)
 - rust-lang#73440 (Add src/librustdoc as an alias for src/tools/rustdoc)
 - rust-lang#73442 (pretty/mir: const value enums with no variants)
 - rust-lang#73452 (Unify region variables when projecting associated types)
 - rust-lang#73458 (Use alloc::Layout in DroplessArena API)
 - rust-lang#73484 (Update the doc for std::prelude to the correct behavior)
 - rust-lang#73506 (Bump Rustfmt and RLS)

Failed merges:

r? @ghost
@bors bors merged commit 17b80d9 into rust-lang:master Jun 20, 2020
@tmiasko tmiasko deleted the incompatible-sanitizers branch June 20, 2020 07:51
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cuviper cuviper

Labels
ICEBreaker-LLVM Bugs identified for the LLVM ICE-breaker group S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
7 participants
@tmiasko @rust-highfive @petrochenkov @rustbot @nikic @bors @cuviper