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.

Sign up for GitHub

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

Jump to bottom

Provide option for specifying the profiler runtime #85284

Merged
merged 2 commits into from
Jun 18, 2021
Merged

Provide option for specifying the profiler runtime #85284

merged 2 commits into from
Jun 18, 2021

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented May 14, 2021

Currently, if -Zinstrument-coverage is enabled, the target is linked
against the library/profiler_builtins crate (which pulls in LLVM's
compiler-rt runtime).

This option enables backends to specify an alternative runtime crate for
handling injected instrumentation calls.

@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 May 14, 2021
@eggyal
Provide option for specifying the profiler runtime
93c6362 
Currently, if `-Zinstrument-coverage` is enabled, the target is linked
against the `library/profiler_builtins` crate (which pulls in LLVM's
compiler-rt runtime).

This option enables backends to specify an alternative runtime crate for
handling injected instrumentation calls.
@eggyal eggyal force-pushed the custom-profiler-runtime branch from 5e5cb17 to 93c6362 Compare May 17, 2021 07:31
jackh726
jackh726 approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is there currently any work being held up elsewhere by this? I.e. is there another profiler runtime ready?

I'm also not sure if this deserves an MCP. I'm not sure how used this option is?

There might be someone better to review this then me (someone more familiar in this area). That being said, the code changes here look mostly fine (modulo one nit). Are there any tests that can be added here?

compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

@eggyal do you know someone who might be a better reviewer? I'm basically okay approving this, if

  1. You don't know of a better reviewer
  2. no_profiler_runtime is in fact probably not used really anywhere (otherwise this probably needs an MCP)
  3. There aren't any tests or such we can add

@jackh726 jackh726 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 Jun 1, 2021
@eggyal
Copy link
Contributor Author

eggyal commented Jun 1, 2021

I have an alternative profiler runtime for an under-development incremental test runner that uses cg_clif, but it's not ready no.

@bjorn3 do you have r+ privileges? Would you be an appropriate reviewer for this? Else perhaps @tmandry or @wesleywiser given that the profiler runtime is part of the coverage instrumentation that I think they oversaw?

@wesleywiser
Copy link
Member

This seems fine to me but let's cc @Amanieu who added -Zno-profiler-runtime.

@Amanieu
Copy link
Member

Amanieu commented Jun 1, 2021 •
edited
Loading

I added that flag for minicov which provides a no_std profiling runtime (based on LLVM's).

Since the profiler runtime is provided by the crate, it just needs rustc/LLVM to emit the profiling instrumentation without injecting a profiling runtime.

@eggyal
Copy link
Contributor Author

eggyal commented Jun 17, 2021

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2021
@jackh726
Copy link
Member

Okay, seems fine to me. @Amanieu I suppose that the docs for minicov will have to be updated to use -Zprofiler-runtime=None (that's how you would specify no runtime right?) instead of -Zno-profiler-runtime).

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2021

📌 Commit 872839e has been approved by jackh726

@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 Jun 17, 2021
@bors
Copy link
Contributor

bors commented Jun 18, 2021

⌛ Testing commit 872839e with merge ed33787...

@bors
Copy link
Contributor

bors commented Jun 18, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing ed33787 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2021
@bors bors merged commit ed33787 into rust-lang:master Jun 18, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 18, 2021
@eggyal eggyal deleted the custom-profiler-runtime branch June 27, 2021 20:53
Amanieu added a commit to Amanieu/rust that referenced this pull request Aug 4, 2021
@Amanieu
Add back -Zno-profiler-runtime
1247f9b 
This was removed by rust-lang#85284 in favor of -Zprofiler-runtime=<name>.
However the suggested -Zprofiler-runtime=None doesn't work because
"None" is treated as a crate name.
@Amanieu Amanieu mentioned this pull request Aug 4, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2021
@JohnTitor
Rollup merge of rust-lang#87756 - Amanieu:no_profiler_runtime, r=jack…
0fc0614 
…h726

Add back -Zno-profiler-runtime

This was removed by rust-lang#85284 in favor of `-Zprofiler-runtime=<name>`.However the suggested `-Zprofiler-runtime=None` doesn't work because`None` is treated as a crate name.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2021
@JohnTitor
Rollup merge of rust-lang#87756 - Amanieu:no_profiler_runtime, r=jack…
5a36cdb 
…h726

Add back -Zno-profiler-runtime

This was removed by rust-lang#85284 in favor of `-Zprofiler-runtime=<name>`.However the suggested `-Zprofiler-runtime=None` doesn't work because`None` is treated as a crate name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
8 participants
@eggyal @rust-highfive @jackh726 @wesleywiser @Amanieu @bors @Aaron1011 @rustbot