Address review comments and Windows failure, and make cleaner

richkadel · richkadel · commit ed89e6b83142 · 2021-04-03T18:03:50.000-07:00
diff --git a/src/bootstrap/dist.rs b/src/bootstrap/dist.rs
@@ -1253,7 +1253,7 @@ pub struct RustDemangler {
 }
 
 impl Step for RustDemangler {
-    type Output = GeneratedTarball;
+    type Output = Option<GeneratedTarball>;
     const ONLY_HOSTS: bool = true;
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
@@ -1271,11 +1271,17 @@ impl Step for RustDemangler {
         });
     }
 
-    fn run(self, builder: &Builder<'_>) -> GeneratedTarball {
+    fn run(self, builder: &Builder<'_>) -> Option<GeneratedTarball> {
         let compiler = self.compiler;
         let target = self.target;
         assert!(builder.config.extended);
 
+        // Only build this extended tool if explicitly included in `tools`, or if `profiler = true`
+        let profiler = builder.config.profiler_enabled(target);
+        if !builder.config.tools.as_ref().map_or(profiler, |t| t.contains("rust-demangler")) {
+            return None;
+        }
+
         let rust_demangler = builder
             .ensure(tool::RustDemangler { compiler, target, extra_features: Vec::new() })
             .expect("rust-demangler expected to build - in-tree tool");
@@ -1286,7 +1292,7 @@ impl Step for RustDemangler {
         tarball.is_preview(true);
         tarball.add_file(&rust_demangler, "bin", 0o755);
         tarball.add_legal_and_readme_to("share/doc/rust-demangler");
-        tarball.generate()
+        Some(tarball.generate())
     }
 }
 
@@ -1326,14 +1332,7 @@ impl Step for Extended {
         let rustc_installer = builder.ensure(Rustc { compiler: builder.compiler(stage, target) });
         let cargo_installer = builder.ensure(Cargo { compiler, target });
         let rustfmt_installer = builder.ensure(Rustfmt { compiler, target });
-        let profiler = builder.config.profiler_enabled(target);
-        let install_rust_demangler =
-            builder.config.tools.as_ref().map_or(profiler, |t| t.contains("rust-demangler"));
-        let rust_demangler_installer = if install_rust_demangler {
-            Some(builder.ensure(RustDemangler { compiler, target }))
-        } else {
-            None
-        };
+        let rust_demangler_installer = builder.ensure(RustDemangler { compiler, target });
         let rls_installer = builder.ensure(Rls { compiler, target });
         let rust_analyzer_installer = builder.ensure(RustAnalyzer { compiler, target });
         let llvm_tools_installer = builder.ensure(LlvmTools { target });
@@ -1359,14 +1358,12 @@ impl Step for Extended {
         let mut tarballs = Vec::new();
         tarballs.push(rustc_installer);
         tarballs.push(cargo_installer);
+        tarballs.push(clippy_installer);
+        tarballs.extend(rust_demangler_installer.clone());
         tarballs.extend(rls_installer.clone());
         tarballs.extend(rust_analyzer_installer.clone());
-        tarballs.push(clippy_installer);
         tarballs.extend(miri_installer.clone());
         tarballs.extend(rustfmt_installer.clone());
-        if let Some(rust_demangler_installer) = rust_demangler_installer {
-            tarballs.push(rust_demangler_installer);
-        }
         tarballs.extend(llvm_tools_installer);
         if let Some(analysis_installer) = analysis_installer {
             tarballs.push(analysis_installer);
@@ -1421,6 +1418,9 @@ impl Step for Extended {
 
         let xform = |p: &Path| {
             let mut contents = t!(fs::read_to_string(p));
+            if rust_demangler_installer.is_none() {
+                contents = filter(&contents, "rust-demangler");
+            }
             if rls_installer.is_none() {
                 contents = filter(&contents, "rls");
             }
@@ -1468,11 +1468,10 @@ impl Step for Extended {
             prepare("rust-docs");
             prepare("rust-std");
             prepare("rust-analysis");
-            if install_rust_demangler {
+            prepare("clippy");
+            if rust_demangler_installer.is_some() {
                 prepare("rust-demangler");
             }
-            prepare("clippy");
-
             if rls_installer.is_some() {
                 prepare("rls");
             }
@@ -1520,6 +1519,8 @@ impl Step for Extended {
                     "rust-analyzer-preview".to_string()
                 } else if name == "clippy" {
                     "clippy-preview".to_string()
+                } else if name == "rust-demangler" {
+                    "rust-demangler-preview".to_string()
                 } else if name == "miri" {
                     "miri-preview".to_string()
                 } else {
@@ -1534,12 +1535,12 @@ impl Step for Extended {
             prepare("rustc");
             prepare("cargo");
             prepare("rust-analysis");
-            if install_rust_demangler {
-                prepare("rust-demangler");
-            }
             prepare("rust-docs");
             prepare("rust-std");
             prepare("clippy");
+            if rust_demangler_installer.is_some() {
+                prepare("rust-demangler");
+            }
             if rls_installer.is_some() {
                 prepare("rls");
             }
@@ -1681,7 +1682,7 @@ impl Step for Extended {
                     .arg("-t")
                     .arg(etc.join("msi/remove-duplicates.xsl")),
             );
-            if install_rust_demangler {
+            if rust_demangler_installer.is_some() {
                 builder.run(
                     Command::new(&heat)
                         .current_dir(&exe)
@@ -1773,6 +1774,9 @@ impl Step for Extended {
                     .arg(&input);
                 add_env(builder, &mut cmd, target);
 
+                if rust_demangler_installer.is_some() {
+                    cmd.arg("-dRustDemanglerDir=rust-demangler");
+                }
                 if rls_installer.is_some() {
                     cmd.arg("-dRlsDir=rls");
                 }
@@ -1795,7 +1799,7 @@ impl Step for Extended {
             candle("CargoGroup.wxs".as_ref());
             candle("StdGroup.wxs".as_ref());
             candle("ClippyGroup.wxs".as_ref());
-            if install_rust_demangler {
+            if rust_demangler_installer.is_some() {
                 candle("RustDemanglerGroup.wxs".as_ref());
             }
             if rls_installer.is_some() {
@@ -1844,7 +1848,7 @@ impl Step for Extended {
             if rust_analyzer_installer.is_some() {
                 cmd.arg("RustAnalyzerGroup.wixobj");
             }
-            if install_rust_demangler {
+            if rust_demangler_installer.is_some() {
                 cmd.arg("RustDemanglerGroup.wixobj");
             }
             if miri_installer.is_some() {
diff --git a/src/bootstrap/install.rs b/src/bootstrap/install.rs
@@ -190,18 +190,14 @@ install!((self, builder, _config),
             );
         }
     };
-    RustDemangler,
-        "rust-demangler",
-        Self::should_build(_config),
-        only_hosts: true,
-    {
-        let profiler = builder.config.profiler_enabled(self.target);
-        let install_rust_demangler =
-            builder.config.tools.as_ref().map_or(profiler, |t| t.contains("rust-demangler"));
-        if install_rust_demangler {
-            let tarball = builder.ensure(
-                dist::RustDemangler { compiler: self.compiler, target: self.target }
-            );
+    RustDemangler, "rust-demangler", Self::should_build(_config), only_hosts: true, {
+        // Note: Even though `should_build` may return true for `extended` default tools,
+        // dist::RustDemangler may still return None, unless the target-dependent `profiler` config
+        // is also true, or the `tools` array explicitly includes "rust-demangler".
+        if let Some(tarball) = builder.ensure(dist::RustDemangler {
+            compiler: self.compiler,
+            target: self.target
+        }) {
             install_sh(builder, "rust-demangler", self.compiler.stage, Some(self.target), &tarball);
         } else {
             builder.info(
diff --git a/src/bootstrap/tarball.rs b/src/bootstrap/tarball.rs
@@ -68,7 +68,7 @@ impl OverlayKind {
         match self {
             OverlayKind::Rust => builder.rust_version(),
             OverlayKind::LLVM => builder.rust_version(),
-            OverlayKind::RustDemangler => builder.rust_version(),
+            OverlayKind::RustDemangler => builder.release_num("rust-demangler"),
             OverlayKind::Cargo => {
                 builder.cargo_info.version(builder, &builder.release_num("cargo"))
             }
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
@@ -393,6 +393,9 @@ impl Step for RustDemangler {
         t!(fs::create_dir_all(&dir));
 
         cargo.env("RUST_DEMANGLER_DRIVER_PATH", rust_demangler);
+
+        cargo.arg("--").args(builder.config.cmd.test_args());
+
         cargo.add_rustc_lib_path(builder, compiler);
 
         builder.run(&mut cargo.into());
diff --git a/src/tools/rust-demangler/README.md b/src/tools/rust-demangler/README.md
@@ -1,18 +1,15 @@
 # rust-demangler
 
-Demangles rustc mangled names.
+_Demangles rustc mangled names._
 
-This tool uses the [rustc-demangle](https://crates.io/crates/rustc-demangle)
-crate to convert an input buffer of newline-separated mangled names into their
-demangled translations.
+`rust-demangler` supports the requirements of the [`llvm-cov show -Xdemangler`
+option](https://llvm.org/docs/CommandGuide/llvm-cov.html#cmdoption-llvm-cov-show-xdemangler),
+to perform Rust-specific symbol demangling:
 
-This tool takes a list of mangled names (one per line) on standard input, and
-prints a corresponding list of demangled names. The tool is designed to support
-programs that can leverage a third-party demangler, such as `llvm-cov`, via the
-`-Xdemangler=<path-to-demangler>` option.
+> _The demangler is expected to read a newline-separated list of symbols from
+> stdin and write a newline-separated list of the same length to stdout._
 
-To use `rust-demangler` with `llvm-cov` for example, add the `-Xdemangler=...`
-option:
+To use `rust-demangler` with `llvm-cov` for example:
 
 ```shell
 $ TARGET="${PWD}/build/x86_64-unknown-linux-gnu"
@@ -21,6 +18,16 @@ $ "${TARGET}"/llvm/bin/llvm-cov show \
   --instr-profile=main.profdata ./main --show-line-counts-or-regions
 ```
 
+`rust-demangler` is a Rust "extended tool", used in Rust compiler tests, and
+optionally included in Rust distributions that enable coverage profiling. Symbol
+demangling is implemented using the
+[rustc-demangle](https://crates.io/crates/rustc-demangle) crate.
+
+_(Note, for Rust developers, the third-party tool
+[`rustfilt`](https://crates.io/crates/rustfilt) also supports `llvm-cov` symbol
+demangling. `rustfilt` is a more generalized tool that searches any body of
+text, using pattern matching, to find and demangle Rust symbols.)_
+
 ## License
 
 Rust-demangler is distributed under the terms of both the MIT license and the
diff --git a/src/tools/rust-demangler/tests/lib.rs b/src/tools/rust-demangler/tests/lib.rs
@@ -22,74 +22,63 @@ _RNvC9backtrace3foo.llvm.A5310EB9
 _RNvNtNtNtNtCs92dm3009vxr_4rand4rngs7adapter9reseeding4fork23FORK_HANDLER_REGISTERED.0.0
 ";
 
+const DEMANGLED_OUTPUT: &str = r"
+123foo[0]::bar
+utf8_idents[317d481089b8c8fe]::საჭმელად_გემრიელი_სადილი
+cc[4d6468d6c9fd4bb3]::spawn::{closure#0}::{closure#0}
+<core[846817f741e54dfd]::slice::Iter<u8> as core[846817f741e54dfd]::iter::iterator::Iterator>::rposition::<core[846817f741e54dfd]::slice::memchr::memrchr::{closure#1}>::{closure#0}
+alloc[f15a878b47eb696b]::alloc::box_free::<dyn alloc[f15a878b47eb696b]::boxed::FnBox<(), Output = ()>>
+INtC8arrayvec8ArrayVechKj7b_E
+<const_generic[317d481089b8c8fe]::Unsigned<11: u8>>
+<const_generic[317d481089b8c8fe]::Signed<152: i16>>
+<const_generic[317d481089b8c8fe]::Signed<-11: i8>>
+<const_generic[317d481089b8c8fe]::Bool<false: bool>>
+<const_generic[317d481089b8c8fe]::Bool<true: bool>>
+<const_generic[317d481089b8c8fe]::Char<'v': char>>
+<const_generic[317d481089b8c8fe]::Char<'\n': char>>
+<const_generic[317d481089b8c8fe]::Char<'∂': char>>
+<const_generic[317d481089b8c8fe]::Foo<_>>::foo::FOO
+foo[0]
+foo[0]
+backtrace[0]::foo
+rand[693ea8e72247470f]::rngs::adapter::reseeding::fork::FORK_HANDLER_REGISTERED.0.0
+";
+
+const DEMANGLED_OUTPUT_NO_CRATE_DISAMBIGUATORS: &str = r"
+123foo[0]::bar
+utf8_idents::საჭმელად_გემრიელი_სადილი
+cc::spawn::{closure#0}::{closure#0}
+<core::slice::Iter<u8> as core::iter::iterator::Iterator>::rposition::<core::slice::memchr::memrchr::{closure#1}>::{closure#0}
+alloc::alloc::box_free::<dyn alloc::boxed::FnBox<(), Output = ()>>
+INtC8arrayvec8ArrayVechKj7b_E
+<const_generic::Unsigned<11: u8>>
+<const_generic::Signed<152: i16>>
+<const_generic::Signed<-11: i8>>
+<const_generic::Bool<false: bool>>
+<const_generic::Bool<true: bool>>
+<const_generic::Char<'v': char>>
+<const_generic::Char<'\n': char>>
+<const_generic::Char<'∂': char>>
+<const_generic::Foo<_>>::foo::FOO
+foo[0]
+foo[0]
+backtrace[0]::foo
+rand::rngs::adapter::reseeding::fork::FORK_HANDLER_REGISTERED.0.0
+";
+
 #[test]
 fn test_demangle_lines() {
     let demangled_lines = demangle_lines(MANGLED_INPUT.lines(), None);
-    let mut iter = demangled_lines.iter();
-    assert_eq!("", iter.next().unwrap());
-    assert_eq!("123foo[0]::bar", iter.next().unwrap());
-    assert_eq!("utf8_idents[317d481089b8c8fe]::საჭმელად_გემრიელი_სადილი", iter.next().unwrap());
-    assert_eq!("cc[4d6468d6c9fd4bb3]::spawn::{closure#0}::{closure#0}", iter.next().unwrap());
-    assert_eq!(
-        "<core[846817f741e54dfd]::slice::Iter<u8> as core[846817f741e54dfd]::iter::iterator::Iterator>::rposition::<core[846817f741e54dfd]::slice::memchr::memrchr::{closure#1}>::{closure#0}",
-        iter.next().unwrap()
-    );
-    assert_eq!(
-        "alloc[f15a878b47eb696b]::alloc::box_free::<dyn alloc[f15a878b47eb696b]::boxed::FnBox<(), Output = ()>>",
-        iter.next().unwrap()
-    );
-    assert_eq!("INtC8arrayvec8ArrayVechKj7b_E", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Unsigned<11: u8>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Signed<152: i16>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Signed<-11: i8>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Bool<false: bool>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Bool<true: bool>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Char<'v': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Char<'\\n': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Char<'∂': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic[317d481089b8c8fe]::Foo<_>>::foo::FOO", iter.next().unwrap());
-    assert_eq!("foo[0]", iter.next().unwrap());
-    assert_eq!("foo[0]", iter.next().unwrap());
-    assert_eq!("backtrace[0]::foo", iter.next().unwrap());
-    assert_eq!(
-        "rand[693ea8e72247470f]::rngs::adapter::reseeding::fork::FORK_HANDLER_REGISTERED.0.0",
-        iter.next().unwrap()
-    );
-    assert!(iter.next().is_none());
+    for (expected, actual) in DEMANGLED_OUTPUT.lines().zip(demangled_lines) {
+        assert_eq!(expected, actual);
+    }
 }
 
 #[test]
 fn test_demangle_lines_no_crate_disambiguators() {
     let demangled_lines = demangle_lines(MANGLED_INPUT.lines(), Some(create_disambiguator_re()));
-    let mut iter = demangled_lines.iter();
-    assert_eq!("", iter.next().unwrap());
-    assert_eq!("123foo[0]::bar", iter.next().unwrap());
-    assert_eq!("utf8_idents::საჭმელად_გემრიელი_სადილი", iter.next().unwrap());
-    assert_eq!("cc::spawn::{closure#0}::{closure#0}", iter.next().unwrap());
-    assert_eq!(
-        "<core::slice::Iter<u8> as core::iter::iterator::Iterator>::rposition::<core::slice::memchr::memrchr::{closure#1}>::{closure#0}",
-        iter.next().unwrap()
-    );
-    assert_eq!(
-        "alloc::alloc::box_free::<dyn alloc::boxed::FnBox<(), Output = ()>>",
-        iter.next().unwrap()
-    );
-    assert_eq!("INtC8arrayvec8ArrayVechKj7b_E", iter.next().unwrap());
-    assert_eq!("<const_generic::Unsigned<11: u8>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Signed<152: i16>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Signed<-11: i8>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Bool<false: bool>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Bool<true: bool>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Char<'v': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Char<'\\n': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Char<'∂': char>>", iter.next().unwrap());
-    assert_eq!("<const_generic::Foo<_>>::foo::FOO", iter.next().unwrap());
-    assert_eq!("foo[0]", iter.next().unwrap());
-    assert_eq!("foo[0]", iter.next().unwrap());
-    assert_eq!("backtrace[0]::foo", iter.next().unwrap());
-    assert_eq!(
-        "rand::rngs::adapter::reseeding::fork::FORK_HANDLER_REGISTERED.0.0",
-        iter.next().unwrap()
-    );
-    assert!(iter.next().is_none());
+    for (expected, actual) in DEMANGLED_OUTPUT_NO_CRATE_DISAMBIGUATORS.lines().zip(demangled_lines)
+    {
+        assert_eq!(expected, actual);
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ impl OverlayKind {`
`68`	`68`	`match self {`
`69`	`69`	`OverlayKind::Rust => builder.rust_version(),`
`70`	`70`	`OverlayKind::LLVM => builder.rust_version(),`
`71`		`- OverlayKind::RustDemangler => builder.rust_version(),`
	`71`	`+ OverlayKind::RustDemangler => builder.release_num("rust-demangler"),`
`72`	`72`	`OverlayKind::Cargo => {`
`73`	`73`	`builder.cargo_info.version(builder, &builder.release_num("cargo"))`
`74`	`74`	`}`