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

Remove core dependency of profiler_builtins #101009

Closed
wants to merge 1 commit into from

Conversation

ldm0
Copy link
Contributor

@ldm0 ldm0 commented Aug 25, 2022

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

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022
@jyn514
Copy link
Member

jyn514 commented Aug 26, 2022

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

@rust-highfive rust-highfive assigned ehuss and unassigned jackh726 Aug 26, 2022
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 26, 2022
library/test/Cargo.toml Outdated Show resolved Hide resolved
@ldm0 ldm0 force-pushed the ldm_fix_profiler_builtins branch from 24d44a1 to 97fb711 Compare August 26, 2022 14:42
@ehuss
Copy link
Contributor

ehuss commented Aug 26, 2022

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 ldm0 force-pushed the ldm_fix_profiler_builtins branch from 1b634a5 to 0865283 Compare August 29, 2022 05:45
@ldm0 ldm0 changed the title Add profiler_builtins to sysroot Remove core dependency of profiler_builtins Aug 29, 2022
@ldm0 ldm0 force-pushed the ldm_fix_profiler_builtins branch from 0865283 to 64e5c40 Compare August 29, 2022 05:49
@ldm0
Copy link
Contributor Author

ldm0 commented Aug 29, 2022

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
Copy link
Contributor Author

ldm0 commented Aug 29, 2022

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)

@ldm0 ldm0 force-pushed the ldm_fix_profiler_builtins branch 3 times, most recently from 042ed64 to de4b44d Compare August 29, 2022 18:12
@ehuss
Copy link
Contributor

ehuss commented Aug 30, 2022

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
Copy link
Member

bjorn3 commented Aug 31, 2022

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
Copy link
Contributor

ehuss commented Aug 31, 2022

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

@ldm0
Copy link
Contributor Author

ldm0 commented Sep 2, 2022

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
Copy link
Contributor

bors commented Sep 3, 2022

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

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2022

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
Copy link
Contributor Author

ldm0 commented Sep 5, 2022

ping @ehuss

@ehuss
Copy link
Contributor

ehuss commented Sep 10, 2022

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
Copy link
Member

bjorn3 commented Sep 10, 2022

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
Copy link
Contributor

ehuss commented Sep 10, 2022

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

@ldm0
Copy link
Contributor Author

ldm0 commented Sep 10, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants