Remove core dependency of profiler_builtins

[ldm0]

This PR removes the core depdency of profiler_builtins, which is not needed.

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

r? @ehuss but feel free to assign to me or Mark if you don't have time

[ehuss]

I'm not sure this will work for build-std. Cargo doesn't build test unless it is needed. But even then, I think profiler_builtins will be needed to be built before core is built.

If the intent is to make it work with an already existing sysroot, I would like to avoid that approach. Ideally we want build-std to work without any pre-installed artifacts.

Also, this seems to remove building profiler_builtins conditionally. Typically that is controlled via the build.profiler setting in config.toml. For example, this will fail if src/llvm-project is not cloned and using download-ci-llvm.

[ldm0]

I'm not sure this will work for build-std. Cargo doesn't build test unless it is needed. But even then, I think profiler_builtins will be needed to be built before core is built.

Ok, I reverted the dependency addition part of this PR.

[ldm0]

Ideally we want build-std to work without any pre-installed artifacts.

I can't find out the config.toml used by compiling distribution version of nightly rust, but the libprofiler_builtins.rlib seems to be already present in the sysroot(build.profiler is enabled?). I guess after this PR, though the referenced problem is not resolved, the problem of using -Cinstrument-coverage and -Cprofile-generate with -Zbuild-std will be mitigated in practice.

(Related links)

• Incompatibility using both -Zinstrument-coverage and -Z build-std #79401
• Support code-coverage wg-cargo-std-aware#63
• Support Profile Guided Optimization (PGO) wg-cargo-std-aware#68

[ehuss]

This still doesn't seem like it will work. When I try to compile for another target, I get can't find crate for `profiler_builtins` . I think maybe there is a cyclic issue where it is trying to inject the profiler runtime while building the profiler runtime. That may work when targeting the host target, as it might pick it out of the host sysroot. However, we would like to avoid needing artifacts available ahead-of-time. That is, we want build-std to work with an empty sysroot as much as possible. Can you show what steps you are taking to test this?

Also, if this definitely is a no_core crate, would it make more sense for it to be a dependency of core itself? If building a no_std crate, I would think it would prevent using the profiler_builtins since they won't get built.

[bjorn3]

Making profiler_builtins a dependency of libcore would cause it to be built even if it isn't used right? And esepcially on targets that don't currently need a C compiler like wasm. You can always build it using -Zbuild-std=core,profiler_builtins, right?

[ehuss]

profiler_builtins is an optional dependency, so it should only be built when the feature is enabled.

[ldm0]

This still doesn't seem like it will work. When I try to compile for another target, I get can't find crate for profiler_builtins . I think maybe there is a cyclic issue where it is trying to inject the profiler runtime while building the profiler runtime. That may work when targeting the host target, as it might pick it out of the host sysroot. However, we would like to avoid needing artifacts available ahead-of-time. That is, we want build-std to work with an empty sysroot as much as possible. Can you show what steps you are taking to test this?

I test this with the host target(x86_64-apple-darwin), and it works by picking out libprofiler_builtins.rlib in the sysroot. I think it won't work on some target without this rlib in sysroot.

Also, if this definitely is a no_core crate, would it make more sense for it to be a dependency of core itself? If building a no_std crate, I would think it would prevent using the profiler_builtins since they won't get built.

I think that make sense. But making profiler_builtins a dependency of core introduces dependency cycle since cc is a build-dependency of profiler_builtins.

Building profiler runtime into something like librustc-*_rt.profiler_builtins.a in bootstrap(just like the sanitizers) might fully resolve the problem. However, this PR shrinks it's scope and won't cause any regression, so that wouldn't be a blocker right? :-/ What do you think? @ehuss

[bors]

☔ The latest upstream changes (presumably #101361) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ldm0]

ping @ehuss

[ehuss]

I think that make sense. But making profiler_builtins a dependency of core introduces dependency cycle since cc is a build-dependency of profiler_builtins.

I'm a little unclear on this point. Isn't cc a build-dependency? Shouldn't that be using the host toolchain while building core? As an alternative, would it help to make it a dependency of alloc similar to compiler_builtins? I don't know why compiler_builtins is structured that way instead of being a dependency of core (the PR that added it (#49503) doesn't give a description of why it was done at all 😦 ).

Building profiler runtime into something like librustc-*_rt.profiler_builtins.a in bootstrap(just like the sanitizers) might fully resolve the problem. However, this PR shrinks it's scope and won't cause any regression, so that wouldn't be a blocker right? :-/ What do you think? @ehuss

I still think depending on having libprofiler_builtins.rlib in the sysroot is the wrong approach for build-std. We've even considered having something like a "no sysroot" flag to rustc to ensure it doesn't try to load anything from there. I would prefer to not move forward with a solution if it isn't what we ultimately want to have, even if it partially solves a need.

[bjorn3]

I don't know why compiler_builtins is structured that way instead of being a dependency of core

Because it depends on libcore for providing all lang items that are necessary for doing anything useful.

[ehuss]

Ah, of course. For some reason I was thinking it was more fundamental than that.

[ldm0]

Building profiler runtime into something like librustc-*_rt.profiler_builtins.a in bootstrap(just like the sanitizers) might fully resolve the problem. However, this PR shrinks it's scope and won't cause any regression, so that wouldn't be a blocker right? :-/ What do you think? @ehuss

I still think depending on having libprofiler_builtins.rlib in the sysroot is the wrong approach for build-std. We've even considered having something like a "no sysroot" flag to rustc to ensure it doesn't try to load anything from there. I would prefer to not move forward with a solution if it isn't what we ultimately want to have, even if it partially solves a need.

Fine, this PR might introduce reliance on libprofiler_builtins.rlib in the sysroot. I'll close this and switch to #101392