From 88b01f1178a86e22742103e8e1f385163e844fbd Mon Sep 17 00:00:00 2001 From: Adam Bratschi-Kaye Date: Thu, 27 May 2021 10:21:53 +0200 Subject: [PATCH] Emit warnings for unused fields in custom targets. --- compiler/rustc_serialize/src/json.rs | 9 + compiler/rustc_session/src/config.rs | 13 +- compiler/rustc_session/src/session.rs | 5 +- compiler/rustc_target/src/lib.rs | 3 + compiler/rustc_target/src/spec/mod.rs | 235 +++++++++++------- .../rustc_target/src/spec/tests/tests_impl.rs | 2 +- compiler/rustc_target/src/tests.rs | 59 +++++ 7 files changed, 235 insertions(+), 91 deletions(-) create mode 100644 compiler/rustc_target/src/tests.rs diff --git a/compiler/rustc_serialize/src/json.rs b/compiler/rustc_serialize/src/json.rs index b79adb6f7bcd9..4d213207fb625 100644 --- a/compiler/rustc_serialize/src/json.rs +++ b/compiler/rustc_serialize/src/json.rs @@ -1114,6 +1114,15 @@ impl Json { } } + /// If the Json value is an Object, deletes the value associated with the + /// provided key from the Object and returns it. Otherwise, returns None. + pub fn remove_key(&mut self, key: &str) -> Option { + match *self { + Json::Object(ref mut map) => map.remove(key), + _ => None, + } + } + /// Attempts to get a nested Json Object for each key in `keys`. /// If any key is found not to exist, `find_path` will return `None`. /// Otherwise, it will return the Json value associated with the final key. diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index af24358fe645f..5afa8e6a09a69 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -12,7 +12,7 @@ use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::impl_stable_hash_via_hash; use rustc_target::abi::{Align, TargetDataLayout}; -use rustc_target::spec::{SplitDebuginfo, Target, TargetTriple}; +use rustc_target::spec::{SplitDebuginfo, Target, TargetTriple, TargetWarnings}; use rustc_serialize::json; @@ -899,9 +899,11 @@ pub(super) fn build_target_config( target_override: Option, sysroot: &PathBuf, ) -> Target { - let target_result = - target_override.map_or_else(|| Target::search(&opts.target_triple, sysroot), Ok); - let target = target_result.unwrap_or_else(|e| { + let target_result = target_override.map_or_else( + || Target::search(&opts.target_triple, sysroot), + |t| Ok((t, TargetWarnings::empty())), + ); + let (target, target_warnings) = target_result.unwrap_or_else(|e| { early_error( opts.error_format, &format!( @@ -911,6 +913,9 @@ pub(super) fn build_target_config( ), ) }); + for warning in target_warnings.warning_messages() { + early_warn(opts.error_format, &warning) + } if !matches!(target.pointer_width, 16 | 32 | 64) { early_error( diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 86b8389a670e6..d8a58ee18cd8e 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -1284,9 +1284,12 @@ pub fn build_session( let target_cfg = config::build_target_config(&sopts, target_override, &sysroot); let host_triple = TargetTriple::from_triple(config::host_triple()); - let host = Target::search(&host_triple, &sysroot).unwrap_or_else(|e| { + let (host, target_warnings) = Target::search(&host_triple, &sysroot).unwrap_or_else(|e| { early_error(sopts.error_format, &format!("Error loading host specification: {}", e)) }); + for warning in target_warnings.warning_messages() { + early_warn(sopts.error_format, &warning) + } let loader = file_loader.unwrap_or_else(|| Box::new(RealFileLoader)); let hash_kind = sopts.debugging_opts.src_hash_algorithm.unwrap_or_else(|| { diff --git a/compiler/rustc_target/src/lib.rs b/compiler/rustc_target/src/lib.rs index cb8f6b9656c68..d39e5a5aa2c3f 100644 --- a/compiler/rustc_target/src/lib.rs +++ b/compiler/rustc_target/src/lib.rs @@ -27,6 +27,9 @@ pub mod abi; pub mod asm; pub mod spec; +#[cfg(test)] +mod tests; + /// Requirements for a `StableHashingContext` to be used in this crate. /// This is a hack to allow using the `HashStable_Generic` derive macro /// instead of implementing everything in `rustc_middle`. diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs index 0f2aaeb533a97..9183908469487 100644 --- a/compiler/rustc_target/src/spec/mod.rs +++ b/compiler/rustc_target/src/spec/mod.rs @@ -905,6 +905,38 @@ supported_targets! { ("bpfel-unknown-none", bpfel_unknown_none), } +/// Warnings encountered when parsing the target `json`. +/// +/// Includes fields that weren't recognized and fields that don't have the expected type. +#[derive(Debug, PartialEq)] +pub struct TargetWarnings { + unused_fields: Vec, + incorrect_type: Vec, +} + +impl TargetWarnings { + pub fn empty() -> Self { + Self { unused_fields: Vec::new(), incorrect_type: Vec::new() } + } + + pub fn warning_messages(&self) -> Vec { + let mut warnings = vec![]; + if !self.unused_fields.is_empty() { + warnings.push(format!( + "target json file contains unused fields: {}", + self.unused_fields.join(", ") + )); + } + if !self.incorrect_type.is_empty() { + warnings.push(format!( + "target json file contains fields whose value doesn't have the correct json type: {}", + self.incorrect_type.join(", ") + )); + } + warnings + } +} + /// Everything `rustc` knows about how to compile for a specific target. /// /// Every field here must be specified, and has no default value. @@ -1447,7 +1479,7 @@ impl Target { } /// Loads a target descriptor from a JSON object. - pub fn from_json(obj: Json) -> Result { + pub fn from_json(mut obj: Json) -> Result<(Target, TargetWarnings), String> { // While ugly, this code must remain this way to retain // compatibility with existing JSON fields and the internal // expected naming of the Target and TargetOptions structs. @@ -1455,10 +1487,9 @@ impl Target { // are round-tripped through this code to catch cases where // the JSON parser is not updated to match the structs. - let get_req_field = |name: &str| { - obj.find(name) - .and_then(Json::as_string) - .map(str::to_string) + let mut get_req_field = |name: &str| { + obj.remove_key(name) + .and_then(|j| Json::as_string(&j).map(str::to_string)) .ok_or_else(|| format!("Field {} in target specification is required", name)) }; @@ -1472,28 +1503,30 @@ impl Target { options: Default::default(), }; + let mut incorrect_type = vec![]; + macro_rules! key { ($key_name:ident) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(s) = obj.find(&name).and_then(Json::as_string) { - base.$key_name = s.to_string(); + if let Some(s) = obj.remove_key(&name).and_then(|j| Json::as_string(&j).map(str::to_string)) { + base.$key_name = s; } } ); ($key_name:ident = $json_name:expr) => ( { let name = $json_name; - if let Some(s) = obj.find(&name).and_then(Json::as_string) { - base.$key_name = s.to_string(); + if let Some(s) = obj.remove_key(&name).and_then(|j| Json::as_string(&j).map(str::to_string)) { + base.$key_name = s; } } ); ($key_name:ident, bool) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(s) = obj.find(&name).and_then(Json::as_boolean) { + if let Some(s) = obj.remove_key(&name).and_then(|j| Json::as_boolean(&j)) { base.$key_name = s; } } ); ($key_name:ident, Option) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(s) = obj.find(&name).and_then(Json::as_u64) { + if let Some(s) = obj.remove_key(&name).and_then(|j| Json::as_u64(&j)) { if s < 1 || s > 5 { return Err("Not a valid DWARF version number".to_string()); } @@ -1502,13 +1535,13 @@ impl Target { } ); ($key_name:ident, Option) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(s) = obj.find(&name).and_then(Json::as_u64) { + if let Some(s) = obj.remove_key(&name).and_then(|j| Json::as_u64(&j)) { base.$key_name = Some(s); } } ); ($key_name:ident, MergeFunctions) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(mergefunc) => base.$key_name = mergefunc, _ => return Some(Err(format!("'{}' is not a valid value for \ @@ -1521,7 +1554,7 @@ impl Target { } ); ($key_name:ident, RelocModel) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(relocation_model) => base.$key_name = relocation_model, _ => return Some(Err(format!("'{}' is not a valid relocation model. \ @@ -1533,7 +1566,7 @@ impl Target { } ); ($key_name:ident, CodeModel) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(code_model) => base.$key_name = Some(code_model), _ => return Some(Err(format!("'{}' is not a valid code model. \ @@ -1545,7 +1578,7 @@ impl Target { } ); ($key_name:ident, TlsModel) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(tls_model) => base.$key_name = tls_model, _ => return Some(Err(format!("'{}' is not a valid TLS model. \ @@ -1557,7 +1590,7 @@ impl Target { } ); ($key_name:ident, PanicStrategy) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s { "unwind" => base.$key_name = PanicStrategy::Unwind, "abort" => base.$key_name = PanicStrategy::Abort, @@ -1570,7 +1603,7 @@ impl Target { } ); ($key_name:ident, RelroLevel) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(level) => base.$key_name = level, _ => return Some(Err(format!("'{}' is not a valid value for \ @@ -1582,7 +1615,7 @@ impl Target { } ); ($key_name:ident, SplitDebuginfo) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(level) => base.$key_name = level, _ => return Some(Err(format!("'{}' is not a valid value for \ @@ -1594,23 +1627,31 @@ impl Target { } ); ($key_name:ident, list) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(v) = obj.find(&name).and_then(Json::as_array) { - base.$key_name = v.iter() - .map(|a| a.as_string().unwrap().to_string()) - .collect(); + if let Some(j) = obj.remove_key(&name){ + if let Some(v) = Json::as_array(&j) { + base.$key_name = v.iter() + .map(|a| a.as_string().unwrap().to_string()) + .collect(); + } else { + incorrect_type.push(name) + } } } ); ($key_name:ident, opt_list) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(v) = obj.find(&name).and_then(Json::as_array) { - base.$key_name = Some(v.iter() - .map(|a| a.as_string().unwrap().to_string()) - .collect()); + if let Some(j) = obj.remove_key(&name) { + if let Some(v) = Json::as_array(&j) { + base.$key_name = Some(v.iter() + .map(|a| a.as_string().unwrap().to_string()) + .collect()); + } else { + incorrect_type.push(name) + } } } ); ($key_name:ident, optional) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(o) = obj.find(&name[..]) { + if let Some(o) = obj.remove_key(&name[..]) { base.$key_name = o .as_string() .map(|s| s.to_string() ); @@ -1618,7 +1659,7 @@ impl Target { } ); ($key_name:ident, LldFlavor) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { if let Some(flavor) = LldFlavor::from_str(&s) { base.$key_name = flavor; } else { @@ -1632,7 +1673,7 @@ impl Target { } ); ($key_name:ident, LinkerFlavor) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match LinkerFlavor::from_str(s) { Some(linker_flavor) => base.$key_name = linker_flavor, _ => return Some(Err(format!("'{}' is not a valid value for linker-flavor. \ @@ -1643,7 +1684,7 @@ impl Target { } ); ($key_name:ident, StackProbeType) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| match StackProbeType::from_json(o) { + obj.remove_key(&name[..]).and_then(|o| match StackProbeType::from_json(&o) { Ok(v) => { base.$key_name = v; Some(Ok(())) @@ -1655,25 +1696,29 @@ impl Target { } ); ($key_name:ident, SanitizerSet) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_array()).and_then(|a| { - for s in a { - base.$key_name |= match s.as_string() { - Some("address") => SanitizerSet::ADDRESS, - Some("leak") => SanitizerSet::LEAK, - Some("memory") => SanitizerSet::MEMORY, - Some("thread") => SanitizerSet::THREAD, - Some("hwaddress") => SanitizerSet::HWADDRESS, - Some(s) => return Some(Err(format!("unknown sanitizer {}", s))), - _ => return Some(Err(format!("not a string: {:?}", s))), - }; + if let Some(o) = obj.remove_key(&name[..]) { + if let Some(a) = o.as_array() { + for s in a { + base.$key_name |= match s.as_string() { + Some("address") => SanitizerSet::ADDRESS, + Some("leak") => SanitizerSet::LEAK, + Some("memory") => SanitizerSet::MEMORY, + Some("thread") => SanitizerSet::THREAD, + Some("hwaddress") => SanitizerSet::HWADDRESS, + Some(s) => return Err(format!("unknown sanitizer {}", s)), + _ => return Err(format!("not a string: {:?}", s)), + }; + } + } else { + incorrect_type.push(name) } - Some(Ok(())) - }).unwrap_or(Ok(())) + } + Ok::<(), String>(()) } ); ($key_name:ident, crt_objects_fallback) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match s.parse::() { Ok(fallback) => base.$key_name = Some(fallback), _ => return Some(Err(format!("'{}' is not a valid CRT objects fallback. \ @@ -1684,7 +1729,7 @@ impl Target { } ); ($key_name:ident, link_objects) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(val) = obj.find(&name[..]) { + if let Some(val) = obj.remove_key(&name[..]) { let obj = val.as_object().ok_or_else(|| format!("{}: expected a \ JSON object with fields per CRT object kind.", name))?; let mut args = CrtObjects::new(); @@ -1712,7 +1757,7 @@ impl Target { } ); ($key_name:ident, link_args) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(val) = obj.find(&name[..]) { + if let Some(val) = obj.remove_key(&name[..]) { let obj = val.as_object().ok_or_else(|| format!("{}: expected a \ JSON object with fields per linker-flavor.", name))?; let mut args = LinkArgs::new(); @@ -1739,22 +1784,26 @@ impl Target { } ); ($key_name:ident, env) => ( { let name = (stringify!($key_name)).replace("_", "-"); - if let Some(a) = obj.find(&name[..]).and_then(|o| o.as_array()) { - for o in a { - if let Some(s) = o.as_string() { - let p = s.split('=').collect::>(); - if p.len() == 2 { - let k = p[0].to_string(); - let v = p[1].to_string(); - base.$key_name.push((k, v)); + if let Some(o) = obj.remove_key(&name[..]) { + if let Some(a) = o.as_array() { + for o in a { + if let Some(s) = o.as_string() { + let p = s.split('=').collect::>(); + if p.len() == 2 { + let k = p[0].to_string(); + let v = p[1].to_string(); + base.$key_name.push((k, v)); + } } } + } else { + incorrect_type.push(name) } } } ); ($key_name:ident, Option) => ( { let name = (stringify!($key_name)).replace("_", "-"); - obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + obj.remove_key(&name[..]).and_then(|o| o.as_string().and_then(|s| { match lookup_abi(s) { Some(abi) => base.$key_name = Some(abi), _ => return Some(Err(format!("'{}' is not a valid value for abi", s))), @@ -1763,20 +1812,26 @@ impl Target { })).unwrap_or(Ok(())) } ); ($key_name:ident, TargetFamilies) => ( { - let value = obj.find("target-family"); - if let Some(v) = value.and_then(Json::as_array) { - base.$key_name = v.iter() - .map(|a| a.as_string().unwrap().to_string()) - .collect(); - } else if let Some(v) = value.and_then(Json::as_string) { - base.$key_name = vec![v.to_string()]; + if let Some(value) = obj.remove_key("target-family") { + if let Some(v) = Json::as_array(&value) { + base.$key_name = v.iter() + .map(|a| a.as_string().unwrap().to_string()) + .collect(); + } else if let Some(v) = Json::as_string(&value) { + base.$key_name = vec![v.to_string()]; + } } } ); } - if let Some(s) = obj.find("target-endian").and_then(Json::as_string) { - base.endian = s.parse()?; + if let Some(j) = obj.remove_key("target-endian") { + if let Some(s) = Json::as_string(&j) { + base.endian = s.parse()?; + } else { + incorrect_type.push("target-endian".to_string()) + } } + key!(is_builtin, bool); key!(c_int_width = "target-c-int-width"); key!(os); @@ -1876,32 +1931,39 @@ impl Target { // NB: The old name is deprecated, but support for it is retained for // compatibility. for name in ["abi-blacklist", "unsupported-abis"].iter() { - if let Some(array) = obj.find(name).and_then(Json::as_array) { - for name in array.iter().filter_map(|abi| abi.as_string()) { - match lookup_abi(name) { - Some(abi) => { - if abi.generic() { + if let Some(j) = obj.remove_key(name) { + if let Some(array) = Json::as_array(&j) { + for name in array.iter().filter_map(|abi| abi.as_string()) { + match lookup_abi(name) { + Some(abi) => { + if abi.generic() { + return Err(format!( + "The ABI \"{}\" is considered to be supported on all \ + targets and cannot be marked unsupported", + abi + )); + } + + base.unsupported_abis.push(abi) + } + None => { return Err(format!( - "The ABI \"{}\" is considered to be supported on all \ - targets and cannot be marked unsupported", - abi + "Unknown ABI \"{}\" in target specification", + name )); } - - base.unsupported_abis.push(abi) - } - None => { - return Err(format!( - "Unknown ABI \"{}\" in target specification", - name - )); } } } } } - Ok(base) + // Each field should have been read using `Json::remove_key` so any keys remaining are unused. + let remaining_keys = obj.as_object().ok_or("Expected JSON object for target")?.keys(); + Ok(( + base, + TargetWarnings { unused_fields: remaining_keys.cloned().collect(), incorrect_type }, + )) } /// Search for a JSON file specifying the given target triple. @@ -1913,12 +1975,15 @@ impl Target { /// /// The error string could come from any of the APIs called, including filesystem access and /// JSON decoding. - pub fn search(target_triple: &TargetTriple, sysroot: &PathBuf) -> Result { + pub fn search( + target_triple: &TargetTriple, + sysroot: &PathBuf, + ) -> Result<(Target, TargetWarnings), String> { use rustc_serialize::json; use std::env; use std::fs; - fn load_file(path: &Path) -> Result { + fn load_file(path: &Path) -> Result<(Target, TargetWarnings), String> { let contents = fs::read(path).map_err(|e| e.to_string())?; let obj = json::from_reader(&mut &contents[..]).map_err(|e| e.to_string())?; Target::from_json(obj) @@ -1928,7 +1993,7 @@ impl Target { TargetTriple::TargetTriple(ref target_triple) => { // check if triple is in list of built-in targets if let Some(t) = load_builtin(target_triple) { - return Ok(t); + return Ok((t, TargetWarnings::empty())); } // search for a file named `target_triple`.json in RUST_TARGET_PATH diff --git a/compiler/rustc_target/src/spec/tests/tests_impl.rs b/compiler/rustc_target/src/spec/tests/tests_impl.rs index f4de8bc0a5803..6730319dcfae2 100644 --- a/compiler/rustc_target/src/spec/tests/tests_impl.rs +++ b/compiler/rustc_target/src/spec/tests/tests_impl.rs @@ -3,7 +3,7 @@ use super::super::*; // Test target self-consistency and JSON encoding/decoding roundtrip. pub(super) fn test_target(target: Target) { target.check_consistency(); - assert_eq!(Target::from_json(target.to_json()), Ok(target)); + assert_eq!(Target::from_json(target.to_json()).map(|(j, _)| j), Ok(target)); } impl Target { diff --git a/compiler/rustc_target/src/tests.rs b/compiler/rustc_target/src/tests.rs new file mode 100644 index 0000000000000..3a737b3355a68 --- /dev/null +++ b/compiler/rustc_target/src/tests.rs @@ -0,0 +1,59 @@ +use crate::spec::Target; +use rustc_serialize::json::Json; +use std::str::FromStr; + +#[test] +fn report_unused_fields() { + let json = Json::from_str( + r#" + { + "arch": "powerpc64", + "data-layout": "e-m:e-i64:64-n32:64", + "llvm-target": "powerpc64le-elf", + "target-pointer-width": "64", + "code-mode": "foo" + } + "#, + ) + .unwrap(); + let warnings = Target::from_json(json).unwrap().1; + assert_eq!(warnings.warning_messages().len(), 1); + assert!(warnings.warning_messages().join("\n").contains("code-mode")); +} + +#[test] +fn report_incorrect_json_type() { + let json = Json::from_str( + r#" + { + "arch": "powerpc64", + "data-layout": "e-m:e-i64:64-n32:64", + "llvm-target": "powerpc64le-elf", + "target-pointer-width": "64", + "link-env-remove": "foo" + } + "#, + ) + .unwrap(); + let warnings = Target::from_json(json).unwrap().1; + assert_eq!(warnings.warning_messages().len(), 1); + assert!(warnings.warning_messages().join("\n").contains("link-env-remove")); +} + +#[test] +fn no_warnings_for_valid_target() { + let json = Json::from_str( + r#" + { + "arch": "powerpc64", + "data-layout": "e-m:e-i64:64-n32:64", + "llvm-target": "powerpc64le-elf", + "target-pointer-width": "64", + "link-env-remove": ["foo"] + } + "#, + ) + .unwrap(); + let warnings = Target::from_json(json).unwrap().1; + assert_eq!(warnings.warning_messages().len(), 0); +}