`build-std` compatible profile(code coverage) support #101392

ldm0 · 2022-09-03T21:21:52Z

Remove the injected profiler_builtins crate (user-provided profiler runtime through -Zprofiler-runtime is still injected)
Compile LLVM's compiler-rt/profile in bootstrap and copy it to sysroot(just like sanitizers)

fixes #79401
fixes rust-lang/wg-cargo-std-aware#63
fixes rust-lang/wg-cargo-std-aware#68

cc @ehuss @bjorn3 @jyn514

rust-highfive · 2022-09-03T21:21:55Z

r? @jyn514

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

compiler/rustc_codegen_ssa/src/back/link.rs

bjorn3 · 2022-09-04T05:52:50Z

Wouldn't this make profiling for targets other than the host impossible with -Zbuild-std?

ldm0 · 2022-09-04T09:44:33Z

Wouldn't this make profiling for targets other than the host impossible with -Zbuild-std?

Do you mean cross compiling with profiling enabled? I think that's possible since profiler runtime is provided in the target lib in sysroot.

This PR makes profiler-runtime to be provided in the same way as santizer-runtimes (#65241 could be referenced). Therefore, profilers can be used in almost the same way as sanitizers.

bjorn3 · 2022-09-04T09:53:57Z

Do you mean cross compiling with profiling enabled?

Yes

I think that's possible since profiler runtime is provided in the target lib in sysroot.

What if you want to use -Zbuild-std because there is no sysroot for the target in the first place?

ldm0 · 2022-09-04T10:22:35Z

I think that's possible since profiler runtime is provided in the target lib in sysroot.

What if you want to use -Zbuild-std because there is no sysroot for the target in the first place?

Currenly without the prebuilt static lib, it's not possible. We can copy the source code with rust-src though. But it would be awkward to compile a cpp based compiler-rt when building std, unnecessary dependencies like cmake or cc will be required.

rustbot · 2022-09-04T17:01:09Z

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

jyn514

How is this PR related to #101009 ? Does it require that PR to merge before? Does it make sense for #101009 to land separately, or should these two be combined?

compiler/rustc_codegen_ssa/src/back/link.rs

ldm0 · 2022-09-07T04:26:39Z

How is this PR related to #101009 ? Does it require that PR to merge before? Does it make sense for #101009 to land separately, or should these two be combined?

#101009 is expected to be a harmless&simple improvement which I expect to merge before this PR.

But If this PR is merged before #101009 , #101009 is useless since profiler_builtins is removed.

ehuss · 2022-09-10T15:24:38Z

I'm a bit confused on how this solves the linked issues. It looks like this depends on having profiler_builtins in the sysroot, which I mentioned in #101009 (comment) is probably not the direction I would want to go. Am I misreading how this works?

ldm0 · 2022-09-10T15:36:26Z

I'm a bit confused on how this solves the linked issues. It looks like this depends on having profiler_builtins in the sysroot, which I mentioned in #101009 (comment) is probably not the direction I would want to go. Am I misreading how this works?

This PR make profiler provided in the same way as sanitizers(prebuilt *.a). They are all compiler-rt in LLVM, if providing sanitizers with prebuilt static libs was acceptable before, providing profiler runtime as static lib should also be acceptable now I guess? Nobody want to run cmake while building a rust project.

Ref: #65241

bjorn3 · 2022-09-10T15:44:44Z

They are all compiler-rt in LLVM, if providing sanitizers with prebuilt static libs was acceptable before, providing profiler runtime as static lib should also be acceptable now I guess?

Sanitizer support was introduced several years before -Zbuild-std. If -Zbuild-std support had been introduced first, it would likely not have been considered acceptable to use precompiled static libs for sanitizers.

Nobody want to run cmake while building a rust project.

We already require a C compiler for building compiler-builtins on many targets. Is cmake that much worse? Would it be possible to rewrite the build logic for the parts of compiler-rt that we need in rust instead if this is the case?

ldm0 · 2022-09-10T18:21:50Z

They are all compiler-rt in LLVM, if providing sanitizers with prebuilt static libs was acceptable before, providing profiler runtime as static lib should also be acceptable now I guess?

Sanitizer support was introduced several years before -Zbuild-std. If -Zbuild-std support had been introduced first, it would likely not have been considered acceptable to use precompiled static libs for sanitizers.

Profiler support was also introduced several years before -Zbuild-std(#42433).
If providing libunwind and sanitizers as static libs in sysroot is acceptable (#65241, #85600), this PR should also be acceptable.

Is cmake that much worse?

Add unnecessary CMake+Cpp dependencies in rust project is practically bad (various compilation issues will occurs) e.g. tokio-rs/prost#657

Would it be possible to rewrite the build logic for the parts of compiler-rt that we need in rust instead if this is the case?

compiler-rt has more than 10 thousand lines of CMake, porting them to Rust doesn't sounds like a good idea(error prone, LLVM upgrading burden, LLVM version locking)

Summary of my concerns:

rust doesn't vendor CMake yet. Introducing dependeny of CMake provided by user to avoid vendored dependencies(libclang_rt.{}_{}.a) might introduce more problems.
profiler_builtins cannot be a dependency of libcore as long as it's a crate with build script needed due to cyclic dependency problem(even if core is only a build-dependency). Providing profiler runtime as static lib effectivly resolves the problem.

Our company project is relying on -Zbuild-std due to enabling of -Zpanic-in-drop=abort, and PGO and instrument-coverage support are all broken by it currently. I would like to argue whether "This PR is not the solution what we ultimately want to have" is a strong enough argument to reject a legit bugfix.

tmiasko · 2022-09-10T20:15:20Z

What use-cases are you trying to address by suggesting this direction @bjorn3?

Using -Zbuild-std with a target other than the host. In that case there is no prebuilt sanitizer or profiler runtime availabe in the sysroot.

As mentioned earlier in this thread. It is already possible, but requires target to be installed.

bjorn3 · 2022-09-10T21:14:19Z

That doesn't help anything for tier3 and external targets as they can't be installed.

tmiasko · 2022-09-11T16:01:57Z

I see, so you are interested in use-cases where those components have to be built because they are otherwise unavailable. I would expect that implementing support for such use-cases would be up to the people interested in them. I don't think we should block -Zbuild-std compatible implementation for that reason. Unless you have concrete proposal for alternative solution?

bjorn3 · 2022-09-11T16:14:08Z

Maybe we could have a precompiled profiler runtime in the sysroot for tier 1 and tier 2 targets, but build it from scratch using cmake if the sysroot version is not available? This could be implemented by making the profiler_builtins crate #![no_std] and in the build script check if the .rlib is available for the target and if so link against it like a staticlib and if not build from scratch. This shouldn't need any rustbuild or cargo changes.

bors · 2022-09-29T05:51:14Z

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

bors · 2022-10-12T21:05:32Z

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

jyn514 · 2022-10-19T18:54:54Z

src/test/ui/feature-gates/feature-gate-profiler-runtime.stderr

@@ -1,4 +1,4 @@
-error[E0658]: the `#[profiler_runtime]` attribute is used to identify the `profiler_builtins` crate which contains the profiler runtime and will never be stable
+error[E0658]: the `#[profiler_runtime]` attribute is used to identify the crate which contains the profiler runtime and will never be stable


Is this attribute still used anywhere? Can we remove it altogether?

There is an unstable option "-Zprofiler-runtime=<crate_name>" for users to inject their custom profiler runtime. The profiler_runtime attribute is the profiler-runtime-crate marker. So if the attribute is removed, -Zprofiler-runtime will be useless.

bors · 2022-10-23T12:06:37Z

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

bors · 2022-10-31T16:14:24Z

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

JohnCSimon · 2023-01-01T19:11:04Z

@ldm0
ping from triage - can you post your status on this PR? Thanks

JohnCSimon · 2023-01-29T07:10:47Z

@ldm0
ping from triage - can you post your status on this PR? Thanks

Dylan-DPC · 2023-05-15T18:45:42Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rust-highfive assigned jyn514 Sep 3, 2022

rustbot added 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. labels Sep 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 3, 2022

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch 3 times, most recently from 255575f to 0a4ee4d Compare September 3, 2022 21:44

This comment has been minimized.

Sign in to view

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch from b70858b to a2a1455 Compare September 4, 2022 04:56

bjorn3 reviewed Sep 4, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch from 7a65dce to 1beefdd Compare September 4, 2022 17:01

jyn514 suggested changes Sep 7, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2022

ldm0 requested a review from jyn514 September 7, 2022 13:15

ldm0 mentioned this pull request Sep 10, 2022

Remove core dependency of profiler_builtins #101009

Closed

jyn514 assigned ehuss and unassigned jyn514 Sep 10, 2022

taiki-e mentioned this pull request Sep 19, 2022

Support for Wasm? taiki-e/cargo-llvm-cov#221

Open

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2022

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch 3 times, most recently from 6663815 to 2e88784 Compare October 5, 2022 17:41

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch from 2e88784 to 98e9732 Compare October 19, 2022 17:47

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 19, 2022

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch from 98e9732 to 081e102 Compare October 19, 2022 18:48

jyn514 reviewed Oct 19, 2022

View reviewed changes

Donough Liu and others added 4 commits October 24, 2022 03:35

Remove depdencies of profiler_builtins

4495cba

Add LLVM compiler-rt/profile in bootstrap

e74143e

Remove profiler_builtins crate

36068ca

Make profiler_runtime Option<String>

04963ab

ldm0 force-pushed the ldm_fix_profiler_builtins_further branch from 081e102 to 04963ab Compare October 23, 2022 19:43

Dylan-DPC closed this May 15, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2023

Ax9DTW mentioned this pull request Jan 3, 2024

Code Coverage Support for Core Library ferrocene/ferrocene#180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`build-std` compatible profile(code coverage) support #101392

`build-std` compatible profile(code coverage) support #101392

ldm0 commented Sep 3, 2022 •

edited

Loading

rust-highfive commented Sep 3, 2022

This comment has been minimized.

bjorn3 commented Sep 4, 2022

ldm0 commented Sep 4, 2022

bjorn3 commented Sep 4, 2022

ldm0 commented Sep 4, 2022 •

edited

Loading

rustbot commented Sep 4, 2022

jyn514 left a comment

ldm0 commented Sep 7, 2022

ehuss commented Sep 10, 2022

ldm0 commented Sep 10, 2022

bjorn3 commented Sep 10, 2022

ldm0 commented Sep 10, 2022

tmiasko commented Sep 10, 2022

bjorn3 commented Sep 10, 2022

tmiasko commented Sep 11, 2022

bjorn3 commented Sep 11, 2022

bors commented Sep 29, 2022

bors commented Oct 12, 2022

jyn514 Oct 19, 2022

ldm0 Oct 23, 2022

bors commented Oct 23, 2022

bors commented Oct 31, 2022

JohnCSimon commented Jan 1, 2023

JohnCSimon commented Jan 29, 2023

Dylan-DPC commented May 15, 2023

		@@ -1,4 +1,4 @@
		error[E0658]: the `#[profiler_runtime]` attribute is used to identify the `profiler_builtins` crate which contains the profiler runtime and will never be stable
		error[E0658]: the `#[profiler_runtime]` attribute is used to identify the crate which contains the profiler runtime and will never be stable

build-std compatible profile(code coverage) support #101392

build-std compatible profile(code coverage) support #101392

Conversation

ldm0 commented Sep 3, 2022 • edited Loading

rust-highfive commented Sep 3, 2022

This comment has been minimized.

bjorn3 commented Sep 4, 2022

ldm0 commented Sep 4, 2022

bjorn3 commented Sep 4, 2022

ldm0 commented Sep 4, 2022 • edited Loading

rustbot commented Sep 4, 2022

jyn514 left a comment

Choose a reason for hiding this comment

ldm0 commented Sep 7, 2022

ehuss commented Sep 10, 2022

ldm0 commented Sep 10, 2022

bjorn3 commented Sep 10, 2022

ldm0 commented Sep 10, 2022

tmiasko commented Sep 10, 2022

bjorn3 commented Sep 10, 2022

tmiasko commented Sep 11, 2022

bjorn3 commented Sep 11, 2022

bors commented Sep 29, 2022

bors commented Oct 12, 2022

jyn514 Oct 19, 2022

Choose a reason for hiding this comment

ldm0 Oct 23, 2022

Choose a reason for hiding this comment

bors commented Oct 23, 2022

bors commented Oct 31, 2022

JohnCSimon commented Jan 1, 2023

JohnCSimon commented Jan 29, 2023

Dylan-DPC commented May 15, 2023

`build-std` compatible profile(code coverage) support #101392

`build-std` compatible profile(code coverage) support #101392

ldm0 commented Sep 3, 2022 •

edited

Loading

ldm0 commented Sep 4, 2022 •

edited

Loading