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

compiler: env/path handling fixes #116487

Merged
merged 4 commits into from
Oct 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/build_system/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ enum CodegenBackend {
}

fn main() {
if env::var("RUST_BACKTRACE").is_err() {
if env::var_os("RUST_BACKTRACE").is_none() {
env::set_var("RUST_BACKTRACE", "1");
}
env::set_var("CG_CLIF_DISABLE_INCR_CACHE", "1");
Expand Down
14 changes: 6 additions & 8 deletions compiler/rustc_codegen_ssa/src/back/rpath.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use pathdiff::diff_paths;
use rustc_data_structures::fx::FxHashSet;
use std::env;
use rustc_fs_util::try_canonicalize;
use std::ffi::OsString;
use std::fs;
use std::path::{Path, PathBuf};

pub struct RPathConfig<'a> {
Expand Down Expand Up @@ -82,12 +81,11 @@ fn get_rpath_relative_to_output(config: &mut RPathConfig<'_>, lib: &Path) -> OsS
// Mac doesn't appear to support $ORIGIN
let prefix = if config.is_like_osx { "@loader_path" } else { "$ORIGIN" };

let cwd = env::current_dir().unwrap();
let mut lib = fs::canonicalize(&cwd.join(lib)).unwrap_or_else(|_| cwd.join(lib));
lib.pop(); // strip filename
let mut output = cwd.join(&config.out_filename);
output.pop(); // strip filename
let output = fs::canonicalize(&output).unwrap_or(output);
// Strip filenames
let lib = lib.parent().unwrap();
let output = config.out_filename.parent().unwrap();
let lib = try_canonicalize(lib).unwrap();
let output = try_canonicalize(output).unwrap();
let relative = path_relative_from(&lib, &output)
.unwrap_or_else(|| panic!("couldn't create relative path from {output:?} to {lib:?}"));

Expand Down
31 changes: 17 additions & 14 deletions compiler/rustc_driver_impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1287,17 +1287,21 @@ pub fn ice_path() -> &'static Option<PathBuf> {
if !rustc_feature::UnstableFeatures::from_environment(None).is_nightly_build() {
return None;
}
if let Ok("0") = std::env::var("RUST_BACKTRACE").as_deref() {
if let Some(s) = std::env::var_os("RUST_BACKTRACE") && s == "0" {
Copy link
Member

Choose a reason for hiding this comment

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

There should be no difference here. env::var will return Err in case of a non-UTF-8 value and env::var_os will return a value not comparing equal to "1".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but this avoids the appearance of an unhandled error.

Would you prefer I revert this?

Copy link
Member

@bjorn3 bjorn3 Oct 6, 2023

Choose a reason for hiding this comment

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

Yes, please revert this here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, no - this is different; the old code would treat non-UTF8 paths as invalid and call env::current_dir. This now correctly handles non-unicode paths.

Copy link
Member

Choose a reason for hiding this comment

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

The place where a path is passed in an env var can remain the same, but the rest can be reverted without behavior changes.

Copy link
Contributor Author

@tamird tamird Oct 6, 2023

Choose a reason for hiding this comment

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

You're right - I went ahead and changed all interactions with RUST_BACKTRACE to env_os for consistency and updated the commit message.

return None;
}
let mut path = match std::env::var("RUSTC_ICE").as_deref() {
// Explicitly opting out of writing ICEs to disk.
Ok("0") => return None,
Ok(s) => PathBuf::from(s),
Err(_) => std::env::current_dir().unwrap_or_default(),
let mut path = match std::env::var_os("RUSTC_ICE") {
Some(s) => {
if s == "0" {
// Explicitly opting out of writing ICEs to disk.
return None;
}
PathBuf::from(s)
}
None => std::env::current_dir().unwrap_or_default(),
};
let now: OffsetDateTime = SystemTime::now().into();
let file_now = now.format(&Rfc3339).unwrap_or(String::new());
let file_now = now.format(&Rfc3339).unwrap_or_default();
let pid = std::process::id();
path.push(format!("rustc-ice-{file_now}-{pid}.txt"));
Some(path)
Expand All @@ -1322,7 +1326,7 @@ pub fn install_ice_hook(bug_report_url: &'static str, extra_info: fn(&Handler))
// by the user. Compiler developers and other rustc users can
// opt in to less-verbose backtraces by manually setting "RUST_BACKTRACE"
// (e.g. `RUST_BACKTRACE=1`)
if std::env::var("RUST_BACKTRACE").is_err() {
if std::env::var_os("RUST_BACKTRACE").is_none() {
std::env::set_var("RUST_BACKTRACE", "full");
}

Expand Down Expand Up @@ -1411,12 +1415,11 @@ pub fn report_ice(info: &panic::PanicInfo<'_>, bug_report_url: &str, extra_info:

static FIRST_PANIC: AtomicBool = AtomicBool::new(true);

let file = if let Some(path) = ice_path().as_ref() {
let file = if let Some(path) = ice_path() {
// Create the ICE dump target file.
match crate::fs::File::options().create(true).append(true).open(&path) {
Ok(mut file) => {
handler
.emit_note(session_diagnostics::IcePath { path: path.display().to_string() });
handler.emit_note(session_diagnostics::IcePath { path: path.clone() });
if FIRST_PANIC.swap(false, Ordering::SeqCst) {
let _ = write!(file, "\n\nrustc version: {version}\nplatform: {triple}");
}
Expand All @@ -1425,10 +1428,10 @@ pub fn report_ice(info: &panic::PanicInfo<'_>, bug_report_url: &str, extra_info:
Err(err) => {
// The path ICE couldn't be written to disk, provide feedback to the user as to why.
handler.emit_warning(session_diagnostics::IcePathError {
path: path.display().to_string(),
path: path.clone(),
error: err.to_string(),
env_var: std::env::var("RUSTC_ICE")
.ok()
env_var: std::env::var_os("RUSTC_ICE")
.map(PathBuf::from)
.map(|env_var| session_diagnostics::IcePathErrorEnv { env_var }),
});
handler.emit_note(session_diagnostics::IceVersion { version, triple });
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_driver_impl/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ pub(crate) struct IceVersion<'a> {
#[derive(Diagnostic)]
#[diag(driver_impl_ice_path)]
pub(crate) struct IcePath {
pub path: String,
pub path: std::path::PathBuf,
}

#[derive(Diagnostic)]
#[diag(driver_impl_ice_path_error)]
pub(crate) struct IcePathError {
pub path: String,
pub path: std::path::PathBuf,
pub error: String,
#[subdiagnostic]
pub env_var: Option<IcePathErrorEnv>,
Expand All @@ -67,7 +67,7 @@ pub(crate) struct IcePathError {
#[derive(Subdiagnostic)]
#[note(driver_impl_ice_path_error_env)]
pub(crate) struct IcePathErrorEnv {
pub env_var: String,
pub env_var: std::path::PathBuf,
}

#[derive(Diagnostic)]
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::svh::Svh;
use rustc_data_structures::sync::{FreezeReadGuard, FreezeWriteGuard};
use rustc_expand::base::SyntaxExtension;
use rustc_fs_util::try_canonicalize;
use rustc_hir::def_id::{CrateNum, LocalDefId, StableCrateId, StableCrateIdMap, LOCAL_CRATE};
use rustc_hir::definitions::Definitions;
use rustc_index::IndexVec;
Expand All @@ -31,7 +32,7 @@ use std::error::Error;
use std::ops::Fn;
use std::path::Path;
use std::time::Duration;
use std::{cmp, env, iter};
use std::{cmp, iter};

pub struct CStore {
metadata_loader: Box<MetadataLoaderDyn>,
Expand Down Expand Up @@ -677,7 +678,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
stable_crate_id: StableCrateId,
) -> Result<&'static [ProcMacro], CrateError> {
// Make sure the path contains a / or the linker will search for it.
let path = env::current_dir().unwrap().join(path);
let path = try_canonicalize(path).unwrap();
let lib = load_dylib(&path, 5).map_err(|err| CrateError::DlOpen(err))?;

let sym_name = self.sess.generate_proc_macro_decls_symbol(stable_crate_id);
Expand Down
Loading