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

Don't build the rust-demangler binary for coverage tests #125816

Merged
merged 4 commits into from
May 31, 2024

Conversation

Zalathar
Copy link
Contributor

The coverage-run tests invoke llvm-cov, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names.

Historically this used src/tools/rust-demangler, which means that we currently build two different command-line tools to help with the coverage tests (rust-demangler and coverage-dump).

However, it occurred to me that if we add a demangler mode to coverage-dump (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build rust-demangler at all.


Note that the rust-demangler binary is separate from the rustc-demangle crate (which both rust-demangler and coverage-dump use as a dependency to do the actual demangling).


So the main benefits/motivations here are:

  • Slightly faster builds after a fresh checkout or bootstrap bump.
  • Making it clear that currently no tests actually need the rust-demangler binary, since the coverage tests can use their own tool instead.

The coverage-dump tool already needs `rustc_demangle` for its own purposes, so
the amount of extra code needed for a demangle mode is very small.
This avoids the need to build `rust-demangler` when running coverage tests,
since we typically need to build `coverage-dump` anyway.
This appears to be the canonical way to build a tool with the stage 0 compiler.
@rustbot
Copy link
Collaborator

rustbot commented May 31, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 31, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 31, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@jieyouxu
Copy link
Member

This looks like mostly a T-bootstrap change
r? bootstrap

@rustbot rustbot assigned albertlarsan68 and unassigned oli-obk May 31, 2024
@Zalathar
Copy link
Contributor Author

One of the reasons I want to do this is that the rust-demangler binary is pretty obscure, and I haven't seen anything outside of the coverage tests actually use it. There doesn't seem to be much documentation pointing to it, and I'm not sure how users would even get their hands on it.

We might be able to follow up by getting rid of that binary entirely, since the need for demangling seems to be well-served by rustfilt and by the rustc-demangle library.

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2024

Is the rust-demangler something we ship with releases or can we now rip it out?

@Zalathar
Copy link
Contributor Author

Is the rust-demangler something we ship with releases or can we now rip it out?

I honestly don't know, and I was in the middle of trying to figure out who to even ask.

There does seem to be code in bootstrap for including it in source/binary tarballs, but I have no idea where it ends up, and so far I haven't seen any evidence of anyone actually using it.

@jieyouxu
Copy link
Member

FWIW, the README says

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 crate.

(Note, for Rust developers, the third-party tool 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.)

But IDK how accurate that is now.

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2024

It's not shipped by default in nightly (or other releases). It may be in a component I don't have, but I can't think of which one should have it.

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2024

ok

@bors r+

let's MCP removing the tool separately just for visibility (in the hope if anyone still knows a need for it, they'll speak up)

@bors
Copy link
Contributor

bors commented May 31, 2024

📌 Commit 54b6849 has been approved by oli-obk

It is now in the queue for this repository.

@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 May 31, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request May 31, 2024
Don't build the `rust-demangler` binary for coverage tests

The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names.

Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`).

However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all.

---

Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling).

---

So the main benefits/motivations here are:
- Slightly faster builds after a fresh checkout or bootstrap bump.
- Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#125652 (Revert propagation of drop-live information from Polonius)
 - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc)
 - rust-lang#125752 (run-make: enforce `#[must_use]` and arm command wrappers with drop bombs)
 - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators)
 - rust-lang#125796 (Also InstSimplify `&raw*`)
 - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request May 31, 2024
Don't build the `rust-demangler` binary for coverage tests

The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names.

Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`).

However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all.

---

Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling).

---

So the main benefits/motivations here are:
- Slightly faster builds after a fresh checkout or bootstrap bump.
- Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#125652 (Revert propagation of drop-live information from Polonius)
 - rust-lang#125730 (Apply `x clippy --fix` and `x fmt` on Rustc)
 - rust-lang#125756 (coverage: Optionally instrument the RHS of lazy logical operators)
 - rust-lang#125776 (Stop using `translate_args` in the new solver)
 - rust-lang#125796 (Also InstSimplify `&raw*`)
 - rust-lang#125807 (Also resolve the type of constants, even if we already turned it into an error constant)
 - rust-lang#125816 (Don't build the `rust-demangler` binary for coverage tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit df9cd17 into rust-lang:master May 31, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024
Rollup merge of rust-lang#125816 - Zalathar:demangler, r=oli-obk

Don't build the `rust-demangler` binary for coverage tests

The coverage-run tests invoke `llvm-cov`, which requires us to specify a command-line demangler that it can use to demangle Rust symbol names.

Historically this used `src/tools/rust-demangler`, which means that we currently build two different command-line tools to help with the coverage tests (`rust-demangler` and `coverage-dump`).

However, it occurred to me that if we add a demangler mode to `coverage-dump` (which is only a handful of lines and no extra dependencies), then we only need to build one helper binary for the coverage tests, and there is no need for tests to build `rust-demangler` at all.

---

Note that the `rust-demangler` binary is separate from the `rustc-demangle` crate (which both `rust-demangler` and `coverage-dump` use as a dependency to do the actual demangling).

---

So the main benefits/motivations here are:
- Slightly faster builds after a fresh checkout or bootstrap bump.
- Making it clear that currently no tests actually need the `rust-demangler` binary, since the coverage tests can use their own tool instead.
@rustbot rustbot added this to the 1.80.0 milestone May 31, 2024
@Zalathar Zalathar deleted the demangler branch June 1, 2024 01:19
fmease added a commit to fmease/rust that referenced this pull request Jun 19, 2024
Remove `src/tools/rust-demangler`

`rust-demangler` is a small binary that reads a list of mangled symbols from stdin, demangles them (using the `rustc-demangle` library crate), and prints the demangled symbols to stdout.

It was added as part of the initial implementation of coverage instrumentation in 2020/2021, so that coverage tests could pass it to `llvm-cov --Xdemangler` when generating coverage reports. It has been largely untouched since then.

As of rust-lang#125816 it is no longer used by coverage tests, and has no remaining in-tree uses.

There is code in bootstrap to build and package the demangler, but it's unclear where the resulting binaries actually end up, or whether there's any reasonable way for `rustup` users to obtain them.

---

For users needing a command-line demangler, `rustfilt` exists and is more actively maintained. It's also quite easy to use the `rustc-demangle` library to build a custom command-line demangler if necessary, with only a few lines of code.

The tool's name (`rust-demangler`) is easily confused with the name of the library crate `rustc-demangle`, so removing the tool will eliminate that confusion. There also doesn't appear to be much reason to use `rust-demangler` over `rustfilt`.

---

This PR therefore removes the tool, and removes all of its associated code from bootstrap.

MCP filed: rust-lang/compiler-team#754
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#125880 - Zalathar:demangler, r=oli-obk

Remove `src/tools/rust-demangler`

`rust-demangler` is a small binary that reads a list of mangled symbols from stdin, demangles them (using the `rustc-demangle` library crate), and prints the demangled symbols to stdout.

It was added as part of the initial implementation of coverage instrumentation in 2020/2021, so that coverage tests could pass it to `llvm-cov --Xdemangler` when generating coverage reports. It has been largely untouched since then.

As of rust-lang#125816 it is no longer used by coverage tests, and has no remaining in-tree uses.

There is code in bootstrap to build and package the demangler, but it's unclear where the resulting binaries actually end up, or whether there's any reasonable way for `rustup` users to obtain them.

---

For users needing a command-line demangler, `rustfilt` exists and is more actively maintained. It's also quite easy to use the `rustc-demangle` library to build a custom command-line demangler if necessary, with only a few lines of code.

The tool's name (`rust-demangler`) is easily confused with the name of the library crate `rustc-demangle`, so removing the tool will eliminate that confusion. There also doesn't appear to be much reason to use `rust-demangler` over `rustfilt`.

---

This PR therefore removes the tool, and removes all of its associated code from bootstrap.

MCP filed: rust-lang/compiler-team#754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants