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

Make debug_triple depend on target json file content rather than file path #98225

Merged
merged 2 commits into from
Jun 20, 2022
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
124 changes: 99 additions & 25 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ use crate::json::{Json, ToJson};
use crate::spec::abi::{lookup as lookup_abi, Abi};
use crate::spec::crt_objects::{CrtObjects, CrtObjectsFallback};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
use rustc_span::symbol::{sym, Symbol};
use serde_json::Value;
use std::borrow::Cow;
use std::collections::BTreeMap;
use std::convert::TryFrom;
use std::hash::{Hash, Hasher};
use std::iter::FromIterator;
use std::ops::{Deref, DerefMut};
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -2183,7 +2185,7 @@ impl Target {
TargetTriple::TargetTriple(ref target_triple) => {
load_builtin(target_triple).expect("built-in target")
}
TargetTriple::TargetPath(..) => {
TargetTriple::TargetJson { .. } => {
panic!("built-in targets doens't support target-paths")
}
}
Expand Down Expand Up @@ -2248,11 +2250,9 @@ impl Target {

Err(format!("Could not find specification for target {:?}", target_triple))
}
TargetTriple::TargetPath(ref target_path) => {
if target_path.is_file() {
return load_file(&target_path);
}
Err(format!("Target path {:?} is not a valid file", target_path))
TargetTriple::TargetJson { ref contents, .. } => {
let obj = serde_json::from_str(contents).map_err(|e| e.to_string())?;
Target::from_json(obj)
}
}
}
Expand Down Expand Up @@ -2421,10 +2421,77 @@ impl ToJson for Target {
}

/// Either a target triple string or a path to a JSON file.
#[derive(PartialEq, Clone, Debug, Hash, Encodable, Decodable)]
#[derive(Clone, Debug)]
pub enum TargetTriple {
TargetTriple(String),
TargetPath(PathBuf),
TargetJson {
/// Warning: This field may only be used by rustdoc. Using it anywhere else will lead to
/// inconsistencies as it is discarded during serialization.
path_for_rustdoc: PathBuf,
triple: String,
contents: String,
},
}

// Use a manual implementation to ignore the path field
impl PartialEq for TargetTriple {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(Self::TargetTriple(l0), Self::TargetTriple(r0)) => l0 == r0,
(
Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents },
Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents },
) => l_triple == r_triple && l_contents == r_contents,
Comment on lines +2441 to +2444
Copy link
Member

Choose a reason for hiding this comment

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

Minor style nit, consider:

Suggested change
(
Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents },
Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents },
) => l_triple == r_triple && l_contents == r_contents,
(l@Self::TargetJson { .. }, r@Self::TargetJson { .. }) => {
l.triple == r.triple && l.contents == r.contents
}

or, if the intent was to have this an exhaustive match, maybe something like this (though there isn’t much value in this…):

Suggested change
(
Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents },
Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents },
) => l_triple == r_triple && l_contents == r_contents,
(Self::TargetJson { path_for_rustdoc: _, triple, contents }, r@Self::TargetJson { .. }) => {
triple == r.triple && contents == r.contents
}

Copy link
Member Author

Choose a reason for hiding this comment

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

or, if the intent was to have this an exhaustive match

Indeed

maybe something like this

Rustfmt puts that suggestion on four lines too.

_ => false,
}
}
}

// Use a manual implementation to ignore the path field
impl Hash for TargetTriple {
fn hash<H: Hasher>(&self, state: &mut H) -> () {
match self {
TargetTriple::TargetTriple(triple) => {
0u8.hash(state);
triple.hash(state)
}
TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents } => {
1u8.hash(state);
triple.hash(state);
contents.hash(state)
}
}
}
}

// Use a manual implementation to prevent encoding the target json file path in the crate metadata
impl<S: Encoder> Encodable<S> for TargetTriple {
fn encode(&self, s: &mut S) {
match self {
TargetTriple::TargetTriple(triple) => s.emit_enum_variant(0, |s| s.emit_str(triple)),
TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents } => s
.emit_enum_variant(1, |s| {
s.emit_str(triple);
s.emit_str(contents)
}),
}
}
}

impl<D: Decoder> Decodable<D> for TargetTriple {
fn decode(d: &mut D) -> Self {
match d.read_usize() {
Copy link
Member

Choose a reason for hiding this comment

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

This is a little sketchy. This could fail if the implementation of the encoder has overriden emit_enum_variant to output something else than usize… Though of course Decoder does not provide a better way 🤷

0 => TargetTriple::TargetTriple(d.read_str().to_owned()),
1 => TargetTriple::TargetJson {
path_for_rustdoc: PathBuf::new(),
triple: d.read_str().to_owned(),
contents: d.read_str().to_owned(),
},
_ => {
panic!("invalid enum variant tag while decoding `TargetTriple`, expected 0..2");
}
}
}
}

impl TargetTriple {
Expand All @@ -2436,20 +2503,28 @@ impl TargetTriple {
/// Creates a target triple from the passed target path.
pub fn from_path(path: &Path) -> Result<Self, io::Error> {
let canonicalized_path = path.canonicalize()?;
Copy link
Member Author

Choose a reason for hiding this comment

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

It should be fine to remove this canonicalize operation now I think. The only difference would be that the triple would be the original file stem rather than the canonicalized one. This may be preferred in case of content addressed storage as the canonicalized name would be a non human readable hash.

Ok(TargetTriple::TargetPath(canonicalized_path))
let contents = std::fs::read_to_string(&canonicalized_path).map_err(|err| {
io::Error::new(
io::ErrorKind::InvalidInput,
format!("target path {:?} is not a valid file: {}", canonicalized_path, err),
)
})?;
let triple = canonicalized_path
.file_stem()
.expect("target path must not be empty")
.to_str()
.expect("target path must be valid unicode")
.to_owned();
Ok(TargetTriple::TargetJson { path_for_rustdoc: canonicalized_path, triple, contents })
}

/// Returns a string triple for this target.
///
/// If this target is a path, the file name (without extension) is returned.
pub fn triple(&self) -> &str {
match *self {
TargetTriple::TargetTriple(ref triple) => triple,
TargetTriple::TargetPath(ref path) => path
.file_stem()
.expect("target path must not be empty")
.to_str()
.expect("target path must be valid unicode"),
TargetTriple::TargetTriple(ref triple)
| TargetTriple::TargetJson { ref triple, .. } => triple,
}
}

Expand All @@ -2459,16 +2534,15 @@ impl TargetTriple {
/// by `triple()`.
pub fn debug_triple(&self) -> String {
use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};

let triple = self.triple();
if let TargetTriple::TargetPath(ref path) = *self {
let mut hasher = DefaultHasher::new();
path.hash(&mut hasher);
let hash = hasher.finish();
format!("{}-{}", triple, hash)
} else {
triple.into()

match self {
TargetTriple::TargetTriple(triple) => triple.to_owned(),
TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents: content } => {
let mut hasher = DefaultHasher::new();
content.hash(&mut hasher);
let hash = hasher.finish();
format!("{}-{}", triple, hash)
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/doctest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,8 @@ fn run_test(
}
compiler.arg("--target").arg(match target {
TargetTriple::TargetTriple(s) => s,
TargetTriple::TargetPath(path) => {
path.to_str().expect("target path must be valid unicode").to_string()
TargetTriple::TargetJson { path_for_rustdoc, .. } => {
path_for_rustdoc.to_str().expect("target path must be valid unicode").to_string()
}
});
if let ErrorOutputType::HumanReadable(kind) = rustdoc_options.error_format {
Expand Down