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

Conditional compilation for sanitizers #66245

Merged
merged 1 commit into from
Dec 2, 2019
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Nov 9, 2019

Configure sanitize option when compiling with a sanitizer to make
it possible to execute different code depending on whether given
sanitizer is enabled or not.

@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 @GuillaumeGomez (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 Nov 9, 2019
@parched
Copy link
Contributor

parched commented Nov 9, 2019

Would this work if you have more than one sanitizer enabled? For example, address and leak sanitizer can be used together (although I'm not sure if rustc currently supports that).

@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 9, 2019

It is possible to configure multiple key-value pairs with the same key, so if
rustc were to support multiple sanitizers at the same time it should be non-issue.

@bors
Copy link
Contributor

bors commented Nov 10, 2019

☔ The latest upstream changes (presumably #66070) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@tmiasko Can you please resolve the merge conflicts?
@GuillaumeGomez is this ready for review?

Thank you

@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 17, 2019

Rebased.

@bors
Copy link
Contributor

bors commented Nov 23, 2019

☔ The latest upstream changes (presumably #66507) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member

r? @oli-obk

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

The implementation looks fine to me. A tracking issue with some links to related work around sanitizers would be good though

------------------------

The `cfg_sanitize` feature makes it possible to execute different code
depending on whether a particular is enabled or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

a particular ... ?

src/librustc/session/config.rs Outdated Show resolved Hide resolved
@tmiasko tmiasko force-pushed the cfg-sanitize branch 2 times, most recently from d7c840f to dc83d94 Compare November 26, 2019 19:32
@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 26, 2019

I used #39699 as a tracking issue.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 27, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit 41477cef469b97439bbb97dcc36d53aa55f494c9 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2019
@bors
Copy link
Contributor

bors commented Nov 27, 2019

⌛ Testing commit 41477cef469b97439bbb97dcc36d53aa55f494c9 with merge 40a2ad9697b29102bd4d637a4076fe384982374f...

@rust-highfive
Copy link
Collaborator

The job x86_64-apple of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-27T12:15:55.6287750Z failures:
2019-11-27T12:15:55.6287820Z 
2019-11-27T12:15:55.6288540Z ---- [ui] ui/sanitize-cfg.rs#leak stdout ----
2019-11-27T12:15:55.6289010Z 
2019-11-27T12:15:55.6289760Z error in revision `leak`: test compilation failed although it shouldn't!
2019-11-27T12:15:55.6289930Z status: exit code: 1
2019-11-27T12:15:55.6291530Z command: "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage2/bin/rustc" "/Users/runner/runners/2.160.1/work/1/s/src/test/ui/sanitize-cfg.rs" "-Zthreads=1" "--target=x86_64-apple-darwin" "--cfg" "leak" "--error-format" "json" "-Zui-testing" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/test/ui/sanitize-cfg.leak" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/native/rust-test-helpers" "-Zsanitizer=leak" "--cfg" "leak" "-L" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/test/ui/sanitize-cfg.leak/auxiliary" "-A" "unused"
2019-11-27T12:15:55.6292630Z ------------------------------------------
2019-11-27T12:15:55.6292730Z 
2019-11-27T12:15:55.6293410Z ------------------------------------------
2019-11-27T12:15:55.6293540Z stderr:
2019-11-27T12:15:55.6293540Z stderr:
2019-11-27T12:15:55.6294220Z ------------------------------------------
2019-11-27T12:15:55.6294960Z error: LeakSanitizer only works with the `x86_64-unknown-linux-gnu` target
2019-11-27T12:15:55.6295140Z error: aborting due to previous error
2019-11-27T12:15:55.6295210Z 
2019-11-27T12:15:55.6295260Z 
2019-11-27T12:15:55.6295930Z ------------------------------------------
2019-11-27T12:15:55.6295930Z ------------------------------------------
2019-11-27T12:15:55.6296010Z 
2019-11-27T12:15:55.6296050Z 
2019-11-27T12:15:55.6296720Z ---- [ui] ui/sanitize-cfg.rs#memory stdout ----
2019-11-27T12:15:55.6296800Z 
2019-11-27T12:15:55.6297500Z error in revision `memory`: test compilation failed although it shouldn't!
2019-11-27T12:15:55.6297630Z status: exit code: 1
2019-11-27T12:15:55.6299230Z command: "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage2/bin/rustc" "/Users/runner/runners/2.160.1/work/1/s/src/test/ui/sanitize-cfg.rs" "-Zthreads=1" "--target=x86_64-apple-darwin" "--cfg" "memory" "--error-format" "json" "-Zui-testing" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/test/ui/sanitize-cfg.memory" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/native/rust-test-helpers" "-Zsanitizer=memory" "--cfg" "memory" "-L" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/test/ui/sanitize-cfg.memory/auxiliary" "-A" "unused"
2019-11-27T12:15:55.6300340Z ------------------------------------------
2019-11-27T12:15:55.6300420Z 
2019-11-27T12:15:55.6301070Z ------------------------------------------
2019-11-27T12:15:55.6301200Z stderr:
2019-11-27T12:15:55.6301200Z stderr:
2019-11-27T12:15:55.6301960Z ------------------------------------------
2019-11-27T12:15:55.6302910Z error: MemorySanitizer only works with the `x86_64-unknown-linux-gnu` target
2019-11-27T12:15:55.6303240Z error: aborting due to previous error
2019-11-27T12:15:55.6303600Z 
2019-11-27T12:15:55.6303750Z 
2019-11-27T12:15:55.6304660Z ------------------------------------------
---
2019-11-27T12:15:55.6439510Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:537:22
2019-11-27T12:15:55.6439780Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-11-27T12:15:55.6461730Z 
2019-11-27T12:15:55.6461870Z 
2019-11-27T12:15:55.6465740Z command did not execute successfully: "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage0-tools-bin/compiletest" "--compile-lib-path" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage2/lib" "--run-lib-path" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage2/lib/rustlib/x86_64-apple-darwin/lib" "--rustc-path" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/stage2/bin/rustc" "--src-base" "/Users/runner/runners/2.160.1/work/1/s/src/test/ui" "--build-base" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/test/ui" "--stage-id" "stage2-x86_64-apple-darwin" "--mode" "ui" "--target" "x86_64-apple-darwin" "--host" "x86_64-apple-darwin" "--llvm-filecheck" "/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/llvm/build/bin/FileCheck" "--nodejs" "/usr/local/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/Users/runner/runners/2.160.1/work/1/s/build/x86_64-apple-darwin/native/rust-test-helpers" "--docck-python" "/usr/local/bin/python2.7" "--lldb-python" "/usr/bin/python" "--lldb-version" "lldb-902.0.79.2\n  Swift-4.1\n" "--lldb-python-dir" "/Applications/Xcode_9.3.app/Contents/SharedFrameworks/LLDB.framework/Resources/Python" "--llvm-version" "9.0.0-rust-1.41.0-dev\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-11-27T12:15:55.6467220Z 
2019-11-27T12:15:55.6467270Z 
2019-11-27T12:15:55.6477090Z failed to run: /Users/runner/runners/2.160.1/work/1/s/build/bootstrap/debug/bootstrap test
2019-11-27T12:15:55.6477230Z Build completed unsuccessfully in 1:00:13
2019-11-27T12:15:55.6477230Z Build completed unsuccessfully in 1:00:13
2019-11-27T12:15:55.6548510Z == clock drift check ==
2019-11-27T12:15:55.6609680Z   local time: Wed Nov 27 12:15:55 UTC 2019
2019-11-27T12:15:55.7027030Z   network time: Wed, 27 Nov 2019 12:15:55 GMT
2019-11-27T12:15:55.7029590Z == end clock drift check ==
2019-11-27T12:15:55.7078380Z 
2019-11-27T12:15:55.7215620Z ##[error]Bash exited with code '1'.
2019-11-27T12:15:55.7266540Z ##[section]Starting: Checkout
2019-11-27T12:15:55.7269920Z ==============================================================================
2019-11-27T12:15:55.7270040Z Task         : Get sources
2019-11-27T12:15:55.7270230Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Nov 27, 2019

💔 Test failed - checks-azure

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 27, 2019
@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 Nov 30, 2019
@bors
Copy link
Contributor

bors commented Dec 1, 2019

☔ The latest upstream changes (presumably #66908) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 1, 2019
Configure sanitize option when compiling with a sanitizer to make
it possible to execute different code depending on whether given
sanitizer is enabled or not.
@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 1, 2019

Rebased.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 1, 2019

📌 Commit c703ff2 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Dec 1, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2019
@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

It is possible to configure multiple key-value pairs with the same key, so if
rustc were to support multiple sanitizers at the same time it should be non-issue.

Is there precedent for that, i.e., is there any existing cfg flag that uses this? It seems quite counter-intuitive that cfg(sanitizer = "a") and cfg(sanitizer = "b") can both be true. That looks like it implies "a" = "b".

@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 1, 2019

A few examples: feature = "name" (configured by cargo), target_feature = "name", target_has_atomic = "size".

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

Hm, you are right. I guess that ship has sailed. Never mind then!

RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
Conditional compilation for sanitizers

Configure sanitize option when compiling with a sanitizer to make
it possible to execute different code depending on whether given
sanitizer is enabled or not.
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
Conditional compilation for sanitizers

Configure sanitize option when compiling with a sanitizer to make
it possible to execute different code depending on whether given
sanitizer is enabled or not.
@bors
Copy link
Contributor

bors commented Dec 2, 2019

⌛ Testing commit c703ff2 with merge 3713ae778689813f687db44c370edbe028912552...

RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
Conditional compilation for sanitizers

Configure sanitize option when compiling with a sanitizer to make
it possible to execute different code depending on whether given
sanitizer is enabled or not.
@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2019

@bors retry
rolled up

bors added a commit that referenced this pull request Dec 2, 2019
Rollup of 5 pull requests

Successful merges:

 - #66245 (Conditional compilation for sanitizers)
 - #66654 (Handle const-checks for `&mut` outside of `HasMutInterior`)
 - #66822 (libunwind_panic: adjust miri panic hack)
 - #66827 (handle diverging functions forwarding their return place)
 - #66834 (rustbuild fixes)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Dec 2, 2019

⌛ Testing commit c703ff2 with merge 4af3ee8...

@bors bors merged commit c703ff2 into rust-lang:master Dec 2, 2019
@tmiasko tmiasko deleted the cfg-sanitize branch March 1, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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