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

Fix sysroot option not being honored across rustc #80933

Merged
merged 1 commit into from
Jan 25, 2021
Merged

Fix sysroot option not being honored across rustc #80933

merged 1 commit into from
Jan 25, 2021

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Jan 12, 2021

Change link_sanitizer_runtime() to check if the sanitizer library exists in the specified/session sysroot, and if it doesn't exist, use the default sysroot. (See #79253.)

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Jan 12, 2021
@rcvalle
Copy link
Member Author

rcvalle commented Jan 12, 2021

r? @Mark-Simulacrum

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 12, 2021

Is there a way to test this?

@rcvalle
Copy link
Member Author

rcvalle commented Jan 12, 2021

It can be tested it by moving the libraries that come with the Rust distribution (including the sanitizer libraries) to a different location, and compiling a program using the -Zsanitizer option (e.g., -Zsanitizer=address). Before the fix, the build fails trying to link the sanitizer libraries from the same location regardless whether sysroot was specified; after the fix, the build succeeds if sysroot is specified to the (sysroot equivalent relative to the) location of the previously moved libraries that come with the Rust distribution and sanitizer libraries.

@nagisa
Copy link
Member

nagisa commented Jan 13, 2021

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Jan 13, 2021

📌 Commit 8c16ad5de78883c2600072ec8d74abace5dd9fe4 has been approved by nagisa

@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 Jan 13, 2021
@nagisa
Copy link
Member

nagisa commented Jan 13, 2021

(Approved this even without the test because it seems super convoluted to test for, unlikely to regress, and the change looks correct without much doubt anyway)

@bjorn3
Copy link
Member

bjorn3 commented Jan 13, 2021

@bors r-

The reason this uses the default sysroot instead of --sysroot is probably that when using a sanitizer the sysroot passed using --sysroot is often build using xargo or cargo-xbuild to ensure that the sysroot is also build with the sanitizer enabled. Neither of which build the sanitizer runtime. Instead the sanitizer runtime from the default sysroot is used.

@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 Jan 13, 2021
@nagisa
Copy link
Member

nagisa commented Jan 13, 2021

Uh, using the default sysroot regardless of --sysroot specified sounds like a wrong way to solve that problem, to be honest. But I guess it can still be accommodated for by having rustc look in multiple locations in some order?

@tmiasko
Copy link
Contributor

tmiasko commented Jan 13, 2021

For the motivation behind the current implementation see #65241 (comment). Starting the search in the provided sysroot and then falling back to the default one sound fine to me.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_codegen_ssa/src/back/link.rs at line 889:
 fn link_sanitizer_runtime(sess: &Session, linker: &mut dyn Linker, name: &str) {
     fn find_sanitizer_runtime(sess: &Session, filename: &String) -> PathBuf {
         let session_tlib =
-            filesearch::make_target_lib_path(&sess.sysroot,
-                sess.opts.target_triple.triple());
+            filesearch::make_target_lib_path(&sess.sysroot, sess.opts.target_triple.triple());
         let path = session_tlib.join(&filename);
         if path.exists() {
             return session_tlib;
Diff in /checkout/compiler/rustc_codegen_ssa/src/back/link.rs at line 897:
         } else {
             let default_sysroot = filesearch::get_or_default_sysroot();
-            let default_tlib =
-                filesearch::make_target_lib_path(&default_sysroot,
-                    sess.opts.target_triple.triple());
+            let default_tlib = filesearch::make_target_lib_path(
+                &default_sysroot,
+                sess.opts.target_triple.triple(),
+            );
             return default_tlib;
     }
     }
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_codegen_ssa/src/back/link.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
Build completed unsuccessfully in 0:00:14

@rcvalle
Copy link
Member Author

rcvalle commented Jan 20, 2021

I've amended my commit to check if the sanitizer library exists in the specified/session sysroot, and if it doesn't exist, use the default sysroot. What do you think?

@rcvalle
Copy link
Member Author

rcvalle commented Jan 20, 2021

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2021
Change link_sanitizer_runtime() to check if the sanitizer library exists
in the specified/session sysroot, and if it doesn't exist, use the
default sysroot.
@nagisa
Copy link
Member

nagisa commented Jan 22, 2021

@bors r+

Now that it falls back to the old implementation if the prerequisite libraries are not found in the proper sysroot, there should be no regression for xargo/build-std use-cases.

@bors
Copy link
Contributor

bors commented Jan 22, 2021

📌 Commit 7378676 has been approved by nagisa

@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 Jan 22, 2021
@bors
Copy link
Contributor

bors commented Jan 23, 2021

⌛ Testing commit 7378676 with merge 0d7abce4a48a9ef8907159ca96c1be64e762342e...

@bors
Copy link
Contributor

bors commented Jan 23, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 23, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@jyn514
Copy link
Member

jyn514 commented Jan 24, 2021

@bors retry

@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 Jan 24, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2021
…as-schievink

Rollup of 14 pull requests

Successful merges:

 - rust-lang#75180 (Implement Error for &(impl Error))
 - rust-lang#78578 (Permit mutable references in all const contexts)
 - rust-lang#79174 (Make std::future a re-export of core::future)
 - rust-lang#79884 (Replace magic numbers with existing constants)
 - rust-lang#80855 (Expand assert!(expr, args..) to include $crate for hygiene on 2021.)
 - rust-lang#80933 (Fix sysroot option not being honored across rustc)
 - rust-lang#81259 (Replace version_check dependency with own version parsing code)
 - rust-lang#81264 (Add unstable option to control doctest run directory)
 - rust-lang#81279 (Small refactor in typeck)
 - rust-lang#81297 (Don't provide backend_optimization_level query for extern crates)
 - rust-lang#81302 (Fix rendering of stabilization version for trait implementors)
 - rust-lang#81310 (Do not mark unit variants as used when in path pattern)
 - rust-lang#81320 (Make bad shlex parsing a pretty error)
 - rust-lang#81338 (Clean up `dominators_given_rpo`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27abbc2 into rust-lang:master Jan 25, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 25, 2021
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. 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.