-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Don't unconditionally build alloc for no-std
targets
#147168
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
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
// for no-std targets we only compile a few no_std crates | ||
if crates.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also pass dependencies, for the same reasons as the comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment is wrong, not the code. That comment used to be in check::Std
and only really makes sense there. check::Std
has the same behavior here before and after, and AFAICT the behavior is correct: see this comment in std_crates_for_run_make
:
rust/src/bootstrap/src/core/build_steps/compile.rs
Lines 452 to 469 in 8338f9f
/// Resolves standard library crates for `Std::run_make` for any build kind (like check, doc, | |
/// build, clippy, etc.). | |
pub fn std_crates_for_run_make(run: &RunConfig<'_>) -> Vec<String> { | |
let mut crates = run.make_run_crates(builder::Alias::Library); | |
// For no_std targets, we only want to check core and alloc | |
// Regardless of core/alloc being selected explicitly or via the "library" default alias, | |
// we only want to keep these two crates. | |
// The set of no_std crates should be kept in sync with what `Builder::std_cargo` does. | |
// Note: an alternative design would be to return an enum from this function (Default vs Subset) | |
// of crates. However, several steps currently pass `-p <package>` even if all crates are | |
// selected, because Cargo behaves differently in that case. To keep that behavior without | |
// making further changes, we pre-filter the no-std crates here. | |
let target_is_no_std = run.builder.no_std(run.target).unwrap_or(false); | |
if target_is_no_std { | |
crates.retain(|c| c == "core" || c == "alloc"); | |
} | |
crates |
I'm going to move this comment from std_cargo to check::Std::make_run
.
It's possible for targets to only support `core` and not `alloc`. Instead of building alloc unconditionally, pass a list of crates to build into `std_cargo`, and only pass `-p alloc` if the list of crates wasn't already filtered to a subset. The original use case was to reuse `std_cargo` for a rustc_driver that doesn't emit metadata. But this seems like a reasonable change regardless.
1787: Custom core coverage aggregator (codename: `blanket`) r=Hoverbear a=jyn514 Summary: We know which symbols we care about covering (those in https://public-docs.ferrocene.dev/main/certification/core/subset.html), and we want to ensure we cover exactly those rather than "whichever functions make it into the binary past LTO". 1. Write a rustc_driver which collects a list of those fully-qualified symbols. Emit them as JSON to `build/host/stage1-std/${certified-target}/release/symbol-report.json`. 2. Rather than using llvm-cov to turn our .profdata files into .info, use our new `blanket` tool to generate an in-memory structured data aggregation. 3. Rather than using grcov to display the aggregation, use the HTML reports in our new `blanket` tool to generate `build/${certified-target}/doc/certified-coverage-report.html. This also fixes the main original bug, which was that we were filtering out `libstd.so` when aggregating coverage reports. That is not correct; without that .so the coverage tool doesn't know how to map the binary info back to source info, and reports all the sources as uncovered. Before, using llvm-cov, it would also be reported as "ignored". Now, using blanket, it's correctly reported as uncovered. Doing things in this way allows us to customize the reporting as well as to add integration tests that we actually have all the coverage we expect. This PR does not currently modify `ferrocene/doc/core-certification/src/spreadsheets/subset.tsv`. Long term, I would like to delete that file, along with `ferrocene/tools/coverage-of-subset`, and use the generated HTML report in `blanket` for everything. --- Additionally, this extends bootstrap to support running std's tests and doctests with `-C instrument-coverage`. Unfortuately, `blanket` tends to crash while parsing the .profdata for these because of an upstream bug in `llvm-profparse`. I am going to investigate why that happens in the next few weeks. In the meantime, if you see "Parsing Failed" while running `blanket`, try again but with fewer tests. Doctests work in the following way: - Pass `--persist-doctest-binaries` to rustdoc for std tests whenever bootstrap is passed `--coverage=library` - Load those binaries when generating coverage, so that blanket can parse their DWARF info Std tests are mostly the same as before, except that passing `-L deps` for profiler_builtins has moved to `instrument_coverage` so that we're passing it consistently instead of only for compile::Std. --- Some things to note while reviewing: - This includes all commits from https://github.com/ferrocene/blanket/. Those commits were pulled in as a subtree, and the private repo will be deleted shortly. - This includes two bugfixes for pre-existing issues in bootstrap, which have upstream PRs: rust-lang/rust#147168 and rust-lang/rust#147167 - Upstream has no way to get a fully-qualified crate name, and when [I asked about upstreaming it](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/print.20a.20fully.20qualified.20path.20name.3F/with/541561323) they didn't seem interested. So 7bd8340 forks the compiler to add a way to do that. I don't *believe* this affects our spec, but Ana may know more. - In #1769 Ana and Christian added various changes for doctests and std. ed186f8 adds to those changes so that we can cross-compile std. That commit is not strictly part of symbol-report, but symbol-report needs it in order to not break. - As part of a previous approach, I tried use rustdoc-json in `symbol-report`. I ripped that out because using a rustc_driver was more reliable. But the changes to make it possible to run `x doc certified-api-docs --json` are still present in 265c76c, because that seems useful. I can rip them out if desired. We caught some pre-existing bugs while working on this: - `-C instrument-coverage` breaks most of the TCP and UDP tests, because they can't bind to the socket. It's unclear why this happens. We'll investigate this later. - We pass `-L deps/` to load profiler_builtins. This can cause caching bugs or incorrect behavior if there are multiple versions of profiler_builtins in the deps directory. We plan to switch to `--extern profiler_builtins=/exact/path.rlib` in the future to avoid this. --- To test this, you can run `x test --coverage=library --no-doc library/core --stage 1`, which should print the path to an HTML file that looks like this: <img width="1335" height="879" alt="image" src="https://github.com/user-attachments/assets/18e61df7-dcc6-467a-9832-3f318de784d8" /> Co-authored-by: Jynn Nelson <jynn.nelson@ferrous-systems.com> Co-authored-by: Ana Hobden <operator@hoverbear.org> Co-authored-by: Christian Poveda <christian.poveda@ferrous-systems.com>
1787: Custom core coverage aggregator (codename: `blanket`) r=Hoverbear a=jyn514 Summary: We know which symbols we care about covering (those in https://public-docs.ferrocene.dev/main/certification/core/subset.html), and we want to ensure we cover exactly those rather than "whichever functions make it into the binary past LTO". 1. Write a rustc_driver which collects a list of those fully-qualified symbols. Emit them as JSON to `build/host/stage1-std/${certified-target}/release/symbol-report.json`. 2. Rather than using llvm-cov to turn our .profdata files into .info, use our new `blanket` tool to generate an in-memory structured data aggregation. 3. Rather than using grcov to display the aggregation, use the HTML reports in our new `blanket` tool to generate `build/${certified-target}/doc/certified-coverage-report.html. This also fixes the main original bug, which was that we were filtering out `libstd.so` when aggregating coverage reports. That is not correct; without that .so the coverage tool doesn't know how to map the binary info back to source info, and reports all the sources as uncovered. Before, using llvm-cov, it would also be reported as "ignored". Now, using blanket, it's correctly reported as uncovered. Doing things in this way allows us to customize the reporting as well as to add integration tests that we actually have all the coverage we expect. This PR does not currently modify `ferrocene/doc/core-certification/src/spreadsheets/subset.tsv`. Long term, I would like to delete that file, along with `ferrocene/tools/coverage-of-subset`, and use the generated HTML report in `blanket` for everything. --- Additionally, this extends bootstrap to support running std's tests and doctests with `-C instrument-coverage`. Unfortuately, `blanket` tends to crash while parsing the .profdata for these because of an upstream bug in `llvm-profparse`. I am going to investigate why that happens in the next few weeks. In the meantime, if you see "Parsing Failed" while running `blanket`, try again but with fewer tests. Doctests work in the following way: - Pass `--persist-doctest-binaries` to rustdoc for std tests whenever bootstrap is passed `--coverage=library` - Load those binaries when generating coverage, so that blanket can parse their DWARF info Std tests are mostly the same as before, except that passing `-L deps` for profiler_builtins has moved to `instrument_coverage` so that we're passing it consistently instead of only for compile::Std. --- Some things to note while reviewing: - This includes all commits from https://github.com/ferrocene/blanket/. Those commits were pulled in as a subtree, and the private repo will be deleted shortly. - This includes two bugfixes for pre-existing issues in bootstrap, which have upstream PRs: rust-lang/rust#147168 and rust-lang/rust#147167 - Upstream has no way to get a fully-qualified crate name, and when [I asked about upstreaming it](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/print.20a.20fully.20qualified.20path.20name.3F/with/541561323) they didn't seem interested. So 7bd8340 forks the compiler to add a way to do that. I don't *believe* this affects our spec, but Ana may know more. - In #1769 Ana and Christian added various changes for doctests and std. ed186f8 adds to those changes so that we can cross-compile std. That commit is not strictly part of symbol-report, but symbol-report needs it in order to not break. - As part of a previous approach, I tried use rustdoc-json in `symbol-report`. I ripped that out because using a rustc_driver was more reliable. But the changes to make it possible to run `x doc certified-api-docs --json` are still present in 265c76c, because that seems useful. I can rip them out if desired. We caught some pre-existing bugs while working on this: - `-C instrument-coverage` breaks most of the TCP and UDP tests, because they can't bind to the socket. It's unclear why this happens. We'll investigate this later. - We pass `-L deps/` to load profiler_builtins. This can cause caching bugs or incorrect behavior if there are multiple versions of profiler_builtins in the deps directory. We plan to switch to `--extern profiler_builtins=/exact/path.rlib` in the future to avoid this. --- To test this, you can run `x test --coverage=library --no-doc library/core --stage 1`, which should print the path to an HTML file that looks like this: <img width="1335" height="879" alt="image" src="https://github.com/user-attachments/assets/18e61df7-dcc6-467a-9832-3f318de784d8" /> Co-authored-by: Jynn Nelson <jynn.nelson@ferrous-systems.com> Co-authored-by: Ana Hobden <operator@hoverbear.org> Co-authored-by: Christian Poveda <christian.poveda@ferrous-systems.com>
It's possible for targets to only support
core
and notalloc
. Instead of building alloc unconditionally, pass a list of crates to build intostd_cargo
, and only pass-p alloc
if the list of crates wasn't already filtered to a subset.The original use case was to reuse
std_cargo
for a rustc_driver that doesn't emit metadata. But this seems like a reasonable change regardless.