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

Support #[track_caller] on closures and generators #87064

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jul 11, 2021

Lang team summary

This PR adds support for placing the #[track_caller] attribute on closure and generator expressions. This attribute's addition behaves identically (from a users perspective) to the attribute being placed on the method in impl Fn/FnOnce/FnMut for ... generated by compiler.

The attribute is currently "double" feature gated -- both stmt_expr_attributes (preexisting) and closure_track_caller (newly added) must be enabled in order to place these attributes on closures.

As the Fn* traits lack a #[track_caller] attribute in their definition, caller information does not propagate when invoking closures through dyn Fn*. There is no limitation that this PR adds in supporting this; it can be added in the future.

Implementation details

This is implemented in the same way as for functions - an extra
location argument is appended to the end of the ABI. For closures,
this argument is not part of the 'tupled' argument storing the
parameters - the final closure argument for #[track_caller] closures
is no longer a tuple.

For direct (monomorphized) calls, the necessary support was already
implemented - we just needeed to adjust some assertions around checking
the ABI and argument count to take closures into account.

For calls through a trait object, more work was needed.
When creating a ReifyShim, we need to create a shim
for the trait method (e.g. FnOnce::call_mut) - unlike normal
functions, closures are never invoked directly, and always go through a
trait method.

Additional handling was needed for InstanceDef::ClosureOnceShim. In
order to pass location information throgh a direct (monomorphized) call
to FnOnce::call_once on an FnMut closure, we need to make
ClosureOnceShim aware of #[tracked_caller]. A new field
track_caller is added to ClosureOnceShim - this is used by
InstanceDef::requires_caller location, allowing codegen to
pass through the extra location argument.

Since ClosureOnceShim.track_caller is only used by codegen,
we end up generating two identical MIR shims - one for
track_caller == true, and one for track_caller == false. However,
these two shims are used by the entire crate (i.e. it's two shims total,
not two shims per unique closure), so this shouldn't a big deal.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Jul 11, 2021
@rust-log-analyzer

This comment has been minimized.

@Aaron1011 Aaron1011 added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 11, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2021
@bors
Copy link
Contributor

bors commented Jul 13, 2021

⌛ Trying commit 9ef105d74208a3bd339678a75595d9b9b4144aee with merge 0dc3a7eeb0512b0600339922ece622801ae932f0...

@bors
Copy link
Contributor

bors commented Jul 13, 2021

☀️ Try build successful - checks-actions
Build commit: 0dc3a7eeb0512b0600339922ece622801ae932f0 (0dc3a7eeb0512b0600339922ece622801ae932f0)

@rust-timer
Copy link
Collaborator

Queued 0dc3a7eeb0512b0600339922ece622801ae932f0 with parent ca99e3e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0dc3a7eeb0512b0600339922ece622801ae932f0): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2021
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

The code changes look reasonable.

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/block.rs Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/mod.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 22, 2021

The language team discussed this in the last meeting, and had several thoughts:

A separate feature gate to avoid coupling this would be good -- in particular, while stmt_expr_attributes is unlikely to stabilize, coupling these two seems like a footgun for ourselves in the future.

There was some discussion about the implications this has for future stabilization of the Fn traits (Fn, FnMut, FnOnce). I think the primary concern is that making sure that the behavior for closures matches placing #[track_caller] on the (compiler-generated) trait implementation for the compiler-generated type of the closure. If that is the case here (at least from a language perspective) then the feeling was that this could likely be accepted.

@Aaron1011 Can you adjust or clarify for these two points? I think it could be useful to provide a lang-team oriented summary of the PR, rather than one oriented towards compiler team reviewers -- currently there are some details in the PR description that reference (hopefully) internal constructs to the compiler.

(Please re-nominate once these are clarified).

@Mark-Simulacrum Mark-Simulacrum removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 22, 2021
@Aaron1011 Aaron1011 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 Jul 22, 2021
@Aaron1011
Copy link
Member Author

A separate feature gate to avoid coupling this would be good -- in particular, while stmt_expr_attributes is unlikely to stabilize, coupling these two seems like a footgun for ourselves in the future.

I've added a feature gate closure_track_caller

I think the primary concern is that making sure that the behavior for closures matches placing #[track_caller] on the (compiler-generated) trait implementation for the compiler-generated type of the closure.

That's correct. The Fn/FnOnce/FnMut traits do not have #[track_caller, so calls through a trait object will not propagate caller location information. However, direct (monomorphized) calls behave as if you had written impl FnOnce for MyClosure { #[track_caller] fn call_once() }

@Aaron1011 Aaron1011 added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2021
@bors
Copy link
Contributor

bors commented Jul 27, 2021

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

@Mark-Simulacrum
Copy link
Member

Thanks @Aaron1011!

This was discussed in the last lang team meeting and there was general sentiment of moving ahead, but we wanted to see the PR description updated (e.g., it looks like the feature gate being added is not there yet). It might also be helpful to provide a concise description for T-lang FCP. I've tried to do so in this comment -- maybe you can take that as a base and uplift it into the PR description. The main bit is that I've tried to stay out of compiler internal impl details with this summary, which (AFAICT) do not affect the language surface area.

Once the description is updated it's likely we can kickoff fcp here.


This PR adds support for placing the #[track_caller] attribute on closure and generator expressions. This attribute's addition behaves identically (from a users perspective) to the attribute being placed on the method in impl Fn/FnOnce/FnMut for ... generated by compiler.

The attribute is currently "double" feature gated -- both stmt_expr_attributes (preexisting) and closure_track_caller (newly added) must be enabled in order to place these attributes on closures.

As the Fn* traits lack a track_caller attribute in their definition, caller information does not propagate when invoking closures through dyn Fn*. There is no limitation that this PR adds in supporting this; it can be added in the future.

@Aaron1011
Copy link
Member Author

@estebank Are there any other changes that you'd like me to make?

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 18, 2021
@bors
Copy link
Contributor

bors commented Sep 22, 2021

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

This PR allows applying a `#[track_caller]` attribute to a
closure/generator expression. The attribute as interpreted as applying
to the compiler-generated implementation of the corresponding trait
method (`FnOnce::call_once`, `FnMut::call_mut`, `Fn::call`, or
`Generator::resume`).

This feature does not have its own feature gate - however, it requires
`#![feature(stmt_expr_attributes)]` in order to actually apply
an attribute to a closure or generator.

This is implemented in the same way as for functions - an extra
location argument is appended to the end of the ABI. For closures,
this argument is *not* part of the 'tupled' argument storing the
parameters - the final closure argument for `#[track_caller]` closures
is no longer a tuple.

For direct (monomorphized) calls, the necessary support was already
implemented - we just needeed to adjust some assertions around checking
the ABI and argument count to take closures into account.

For calls through a trait object, more work was needed.
When creating a `ReifyShim`, we need to create a shim
for the trait method (e.g. `FnOnce::call_mut`) - unlike normal
functions, closures are never invoked directly, and always go through a
trait method.

Additional handling was needed for `InstanceDef::ClosureOnceShim`. In
order to pass location information throgh a direct (monomorphized) call
to `FnOnce::call_once` on an `FnMut` closure, we need to make
`ClosureOnceShim` aware of `#[tracked_caller]`. A new field
`track_caller` is added to `ClosureOnceShim` - this is used by
`InstanceDef::requires_caller` location, allowing codegen to
pass through the extra location argument.

Since `ClosureOnceShim.track_caller` is only used by codegen,
we end up generating two identical MIR shims - one for
`track_caller == true`, and one for `track_caller == false`. However,
these two shims are used by the entire crate (i.e. it's two shims total,
not two shims per unique closure), so this shouldn't a big deal.
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2021

📌 Commit 94b19fa has been approved by estebank

@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 Sep 23, 2021
@bors
Copy link
Contributor

bors commented Sep 23, 2021

⌛ Testing commit 94b19fa with merge 0132f82...

@bors
Copy link
Contributor

bors commented Sep 23, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 0132f82 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2021
@bors bors merged commit 0132f82 into rust-lang:master Sep 23, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 23, 2021
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #87064!

Tested on commit 0132f82.
Direct link to PR: #87064

💔 rls on linux: test-pass → test-fail (cc @Xanewok).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 23, 2021
Tested on commit rust-lang/rust@0132f82.
Direct link to PR: <rust-lang/rust#87064>

💔 rls on linux: test-pass → test-fail (cc @Xanewok).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0132f82): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -1.8% on incr-unchanged builds of webrender-wrench)
  • Small regression in instruction counts (up to 0.5% on incr-unchanged builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Sep 23, 2021
@rylev
Copy link
Member

rylev commented Sep 28, 2021

The large majority of performance regressions were quite small and many were historically noisy benchmarks. Looking at the non-noisy benchmarks nothing seemed to stand out as a clear culprit. There were some test cases that indicated large jumps in incr_comp_persist_dep_graph but this did not hold true across all incremental test cases, and nothing in the changes seemed to jump out at me as potentially causing this issue.

@estebank @Aaron1011 anything to add here or should we mark as triaged?

@estebank
Copy link
Contributor

From my end, nothing stands out as interesting.

@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Oct 26, 2021
bors added a commit to rust-lang/miri that referenced this pull request Aug 28, 2023
add tests for track_caller in closures and generators

taken from rust-lang/rust#87064
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 31, 2023
add tests for track_caller in closures and generators

taken from rust-lang#87064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.