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 tail calls in mir via TerminatorKind::TailCall #113128

Merged
merged 15 commits into from
Jul 8, 2024

Conversation

WaffleLapkin
Copy link
Member

This is one of the interesting bits in tail call implementation — MIR support.

This adds a new TerminatorKind which represents a tail call:

    TailCall {
        func: Operand<'tcx>,
        args: Vec<Operand<'tcx>>,
        fn_span: Span,
    },

Structurally this is very similar to a normal Call but is missing a few fields:

  • destination — tail calls don't write to destination, instead they pass caller's destination to the callee (such that eventual return will write to the caller of the function that used tail call)
  • target — similarly to destination tail calls pass the caller's return address to the callee, so there is nothing to do
  • unwind — I think this is applicable too, although it's a bit confusing
  • call_sourcebecome forbids operators and is not created as a lowering of something else; tail calls always come from HIR (at least for now)

It might be helpful to read the interpreter implementation to understand what TailCall means exactly, although I've tried documenting it too.


There are a few FIXME-questions still left, ideally we'd be able to answer them during review ':)


r? @oli-obk
cc @scottmcm @drmeepster @JakobDegen

@WaffleLapkin WaffleLapkin added the F-explicit_tail_calls `#![feature(explicit_tail_calls)]` label Jun 28, 2023
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

let target = self.cfg.start_new_block();
let terminator = TerminatorKind::Drop {
target,
// The caller will handle this if needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is true here (looking at the MIR opt tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think actually the inner loop should be replaced with build_scope_drops. That should handle the unwinds correctly and reduce code duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've picked @drmeepster's fix commit (beepster4096@f79affd) as af3f2f8.

@matthewjasper does the code look correct now?

compiler/rustc_borrowck/src/lib.rs Outdated Show resolved Hide resolved
@@ -129,6 +129,8 @@ impl<'mir, 'tcx> Qualifs<'mir, 'tcx> {
ccx: &'mir ConstCx<'mir, 'tcx>,
tainted_by_errors: Option<ErrorGuaranteed>,
) -> ConstQualifs {
// FIXME(explicit_tail_calls): uhhhh I think we can return without return now, does it change anything
Copy link
Contributor

Choose a reason for hiding this comment

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

Afaict we don't run mir_const_qualif on const fn anymore (we should change it to assert that instead of silently returning the Default for functions and actually running on const fn were it called for them), so you'd only encounter this situation in constants. 😆 do you have tests for const FOO: u32 = 42; const BAR: u32 = become FOO;?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a test for it, yet, will add.

To be clear, the following makes an error like "become outside of function body" (similarly to how return) works:

const FOO: u32 = 42;
const BAR: u32 = become FOO;

And this ICEs in this PR, but will produce "become requires a function call" once we have the checks (I'll make a PR with them soon in parallel with this one):

const FOO: u32 = 42;

fn bar() -> u32 {
    become FOO;
}

@@ -695,7 +697,14 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
self.super_terminator(terminator, location);

match &terminator.kind {
TerminatorKind::Call { func, args, fn_span, call_source, .. } => {
TerminatorKind::Call { func, args, fn_span, .. }
| TerminatorKind::TailCall { func, args, fn_span, .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same situation as above, is this even reachable?

compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/syntax.rs Show resolved Hide resolved
compiler/rustc_mir_build/src/lints.rs Outdated Show resolved Hide resolved
@@ -258,6 +258,9 @@ pub trait ValueAnalysis<'tcx> {
// They would have an effect, but are not allowed in this phase.
bug!("encountered disallowed terminator");
}
TerminatorKind::TailCall { .. } => {
// FIXME(explicit_tail_calls): determine if we need to do something here (probably not)
Copy link
Contributor

Choose a reason for hiding this comment

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

oof, we'll need to investigate the users of handle_call_return to see how to best handle these, but I presume we'll want to treat it similar to a return.

compiler/rustc_mir_transform/src/const_prop.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_transform/src/inline.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/interpret/terminator.rs Outdated Show resolved Hide resolved
@nbdd0121
Copy link
Contributor

  • unwind — I think this is applicable too, although it's a bit confusing

I feel this is a fast track to UB, unless we have MIR validations to ensure become unwindable_function() in non-unwindable function is prohibited.

It might make more sense to have an unwind field but limited to UnwindAction::Continue and UnwindAction::Unreachable?

@WaffleLapkin
Copy link
Member Author

@nbdd0121 but the thing is: tail call can't actually do anything about unwinding since it replaces the current function with the callee. See the interpreter impl where unwind is copied from the current stack frame. I'm not sure what effect could TailCall::unwind have, although I'm probably missing something.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 30, 2023

I think we need to guarantee at the tail call site, that no things with a destructor are still in scope.

@nbdd0121
Copy link
Contributor

If we have nounwind_fn() is the previous frame, then the MIR will be call nounwind_fn() -> [return ..., unwind unreachable], which means the unwind in the code you pointed to will be UnwindAction::Unreachable. If the replacing stack frame can actually unwind, then we can run into UB.

We will need to have checks somewhere to prevent this from happening.

@WaffleLapkin
Copy link
Member Author

@oli-obk this is trivially guaranteed by the drop order (drops are run before the tail call), or did I misunderstood your question?

@WaffleLapkin
Copy link
Member Author

@nbdd0121 right, I see. When can we generate call nounwind_fn() -> [return ..., unwind unreachable]? I.e. where should we account for the fact that tail calls can potentially unwind?

I'll try adding a mir validator check, but it will only allow us to catch problems, not prevent them.

@nbdd0121
Copy link
Contributor

When can we generate call nounwind_fn() -> [return ..., unwind unreachable]? I.e. where should we account for the fact that tail calls can potentially unwind?

The unwindability is part of the function's ABI or codegen attr that is computed by fn_can_unwind function (this couldn't be changed).

To account for this I think we need to prevent a no-unwind function from tail calling an unwindable function. That is, we have to reject:

fn bar() {}

#[rustc_nounwind]
fn foo() {
    become bar();
}

and

// When compiled with `-C panic=abort`

extern "C-unwind" {
    fn bar();
}

extern "C-unwind" unsafe fn foo() {
    become bar();
}

@RalfJung
Copy link
Member

RalfJung commented Jul 1, 2023

The interpreter also needs to be able to catch potential UB here (and we should probably have a Miri test), e.g. when a nounwind function becomes a function pointer and unsafe shenanigans were used to make the function called via that pointer actually unwind.

@bors
Copy link
Contributor

bors commented Jul 2, 2023

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

Comment on lines 102 to 169
// Note that we can't use `pop_stack_frame` as it "executes"
// the goto to the return block, but we don't want to,
// only the tail called function should return to the current
// return block.
Copy link
Member

Choose a reason for hiding this comment

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

pop_stack_frame does a bunch of other stuff as well though that seems relevant, such as calling after_stack_pop.

What if the old function has return type bool, but the callee has return type u8 and returns 2 -- is that UB? I would think so but currently I think that would be missed. (Even if the static checks ensure this can't usually happen, we can always transmute a fn() -> bool to fn() -> u8 to circumvent those checks.)

Copy link
Member

Choose a reason for hiding this comment

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

pop_stack_frame does a bunch of other stuff as well though that seems relevant, such as calling after_stack_pop.

In particular this is relevant for the aliasing model, where protectors are active until the stack frame pops.

This is actually an interesting question for the aliasing model... should protector remain active until before or after the tail call?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if the old function has return type bool, but the callee has return type u8 and returns 2 -- is that UB? I would think so but currently I think that would be missed.

I haven't tested it yet, but I actually think this won't be missed, because the check is in the eval_fn_call:

// Don't forget to check the return type!
if !Self::check_argument_compat(&caller_fn_abi.ret, &callee_fn_abi.ret) {
let callee_ty = format!("{}", callee_fn_abi.ret.layout.ty);
let caller_ty = format!("{}", caller_fn_abi.ret.layout.ty);
throw_ub_custom!(
fluent::const_eval_incompatible_return_types,
callee_ty = callee_ty,
caller_ty = caller_ty,
)
}

But pop_stack_frame indeed does a lot more (like deallocating locals). @RalfJung what do you think would be the best to implement necessary things for tail calls here? Just copy all the necessary bits into eval_terminator (in the TailCall arm)? Should we try sharing similar bits between pop_stack_frame and tail calls (that potentially seems annoying...)? Should we introduce a new hook (M::before_tail_call) (should it default to M::after_stack_pop?)?

In particular this is relevant for the aliasing model, where protectors are active until the stack frame pops.

This is actually an interesting question for the aliasing model... should protector remain active until before or after the tail call?

This is indeed an interesting question... I'd assume protectors remain active until before the tail call, since we basically replace the current stack frame with a new one.

Copy link
Member

@RalfJung RalfJung Aug 7, 2023

Choose a reason for hiding this comment

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

I haven't tested it yet, but I actually think this won't be missed, because the check is in the eval_fn_call:

That check will treat bool and u8 as being compatible, so it won't catch this problem.

Given that the caller basically delegates its return place to the callee, I think it would be reasonable to say that the caller's type no longer matters. However this means we cannot easily turn a tail call into a regular call as the validity requirements may change then.

I'd assume protectors remain active until before the tail call, since we basically replace the current stack frame with a new one.

This depends on whether we allow the compiler to turn tail calls into regular calls, assuming it can know that this won't lead to exploding stack usage.

I'm okay with punting on the aliasing question for now, we should just make sure to track it somewhere.

More interesting is the unwinding question. If a nounwind function tailcalls to a mayunwind function, and then unwinds, that must still be UB. However that might already be taken care of? The new stack frame will inherit the return_to_block info from the old one, in particular the unwind action, so if that says Unreachable we'll still trigger UB as desired. I assume a tail call itself doesn't have an unwind action, it just gets inherited from the current stack frame?

what do you think would be the best to implement necessary things for tail calls here? Just copy all the necessary bits into eval_terminator (in the TailCall arm)? Should we try sharing similar bits between pop_stack_frame and tail calls (that potentially seems annoying...)? Should we introduce a new hook (M::before_tail_call) (should it default to M::after_stack_pop?)?

I think we want a replace_stack_frame function in eval_context.rs right besides push/pop_stack_frame, so all this code is reasonably close to each other. (Maybe we should move all this to a new stack.rs file or so.) And if there are significant parts that are shared with push_stack_frame and pop_stack_frame, that should probably move into helper functions.

Copy link
Member Author

@WaffleLapkin WaffleLapkin Feb 15, 2024

Choose a reason for hiding this comment

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

Given that the caller basically delegates its return place to the callee, I think it would be reasonable to say that the caller's type no longer matters. However this means we cannot easily turn a tail call into a regular call as the validity requirements may change then.

By "caller's type no longer matters" do you mean that in this scenario c is sound? (b definitely isn't though)

fn a() -> u8 { 2 }

fn b() -> bool {
    become transmute::<fn() -> u8, fn() -> bool>(a)();
}

fn c() {
    let _x: u8 = transmute::<fn() -> bool, fn() -> u8>(b)();
}

I'm okay with punting on the aliasing question for now, we should just make sure to track it somewhere.

Can you open an issue to track this?

More interesting is the unwinding question. If a nounwind function tailcalls to a mayunwind function, and then unwinds, that must still be UB. However that might already be taken care of? The new stack frame will inherit the return_to_block info from the old one, in particular the unwind action, so if that says Unreachable we'll still trigger UB as desired. I assume a tail call itself doesn't have an unwind action, it just gets inherited from the current stack frame?

Yes, this sounds correct to me.

I think we want a replace_stack_frame function in eval_context.rs right besides push/pop_stack_frame, so all this code is reasonably close to each other. (Maybe we should move all this to a new stack.rs file or so.) And if there are significant parts that are shared with push_stack_frame and pop_stack_frame, that should probably move into helper functions.

Ok. I will look into implementing this

Copy link
Member

Choose a reason for hiding this comment

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

&prev_frame.return_place,
ret,
unwind,
)?;
Copy link
Member

Choose a reason for hiding this comment

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

Please add tests in src/tools/miri/tests to ensure tail calls work in Miri. You can run those tests with ./x.py test miri --stage 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the test shows that tail calls in fact don't work in miri 😅

Tail calls trip this assertion:

debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame());

I guess this is to be expected since we do not call after_stack_pop which maintains this cache...

@WaffleLapkin WaffleLapkin 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 10, 2023
@bors
Copy link
Contributor

bors commented Jul 12, 2023

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

@Dylan-DPC
Copy link
Member

@WaffleLapkin any updates on this? thanks

@WaffleLapkin
Copy link
Member Author

@Dylan-DPC the labels are accurate: this is waiting on me resolving conflicts and addressing review comments.

@tranquillity-codes tranquillity-codes removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 15, 2024
@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

@bors r=oli-obk,RalfJung

@WaffleLapkin WaffleLapkin reopened this Jul 7, 2024
@WaffleLapkin
Copy link
Member Author

@bors r=oli-obk,RalfJung

@bors
Copy link
Contributor

bors commented Jul 7, 2024

📌 Commit 14e5d5f has been approved by oli-obk,RalfJung

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 7, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jul 8, 2024
…le, r=oli-obk,RalfJung

Support tail calls in mir via `TerminatorKind::TailCall`

This is one of the interesting bits in tail call implementation — MIR support.

This adds a new `TerminatorKind` which represents a tail call:
```rust
    TailCall {
        func: Operand<'tcx>,
        args: Vec<Operand<'tcx>>,
        fn_span: Span,
    },
```

*Structurally* this is very similar to a normal `Call` but is missing a few fields:
- `destination` — tail calls don't write to destination, instead they pass caller's destination to the callee (such that eventual `return` will write to the caller of the function that used tail call)
- `target` — similarly to `destination` tail calls pass the caller's return address to the callee, so there is nothing to do
- `unwind` — I _think_ this is applicable too, although it's a bit confusing
- `call_source` — `become` forbids operators and is not created as a lowering of something else; tail calls always come from HIR (at least for now)

It might be helpful to read the interpreter implementation to understand what `TailCall` means exactly, although I've tried documenting it too.

-----

There are a few `FIXME`-questions still left, ideally we'd be able to answer them during review ':)

-----

r? `@oli-obk`
cc `@scottmcm` `@DrMeepster` `@JakobDegen`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#113128 (Support tail calls in mir via `TerminatorKind::TailCall`)
 - rust-lang#126841 ([`macro_metavar_expr_concat`] Add support for literals)
 - rust-lang#126881 (Make `NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE` a deny-by-default lint in edition 2024)
 - rust-lang#126921 (Give VaList its own home)
 - rust-lang#127276 (rustdoc: Remove OpaqueTy)
 - rust-lang#127367 (Run alloc sync tests)
 - rust-lang#127431 (Use field ident spans directly instead of the full field span in diagnostics on local fields)
 - rust-lang#127437 (Uplift trait ref is knowable into `rustc_next_trait_solver`)
 - rust-lang#127439 (Uplift elaboration into `rustc_type_ir`)
 - rust-lang#127451 (Improve `run-make/output-type-permutations` code and improve `filename_not_in_denylist` API)
 - rust-lang#127452 (Fix intrinsic const parameter counting with `effects`)
 - rust-lang#127459 (rustdoc-json: add type/trait alias tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jul 8, 2024

⌛ Testing commit 14e5d5f with merge 9af6fee...

@bors
Copy link
Contributor

bors commented Jul 8, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk,RalfJung
Pushing 9af6fee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2024
@bors bors merged commit 9af6fee into rust-lang:master Jul 8, 2024
13 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 8, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9af6fee): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.2%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary 2.2%, secondary 8.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [0.8%, 3.8%] 7
Regressions ❌
(secondary)
10.0% [5.3%, 15.1%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) 2.2% [0.8%, 3.8%] 7

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 700.046s -> 699.651s (-0.06%)
Artifact size: 328.35 MiB -> 328.40 MiB (0.02%)

@WaffleLapkin WaffleLapkin deleted the become_trully_unuwuable branch July 8, 2024 08:19
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 11, 2024
…, r=oli-obk,RalfJung

Support tail calls in mir via `TerminatorKind::TailCall`

This is one of the interesting bits in tail call implementation — MIR support.

This adds a new `TerminatorKind` which represents a tail call:
```rust
    TailCall {
        func: Operand<'tcx>,
        args: Vec<Operand<'tcx>>,
        fn_span: Span,
    },
```

*Structurally* this is very similar to a normal `Call` but is missing a few fields:
- `destination` — tail calls don't write to destination, instead they pass caller's destination to the callee (such that eventual `return` will write to the caller of the function that used tail call)
- `target` — similarly to `destination` tail calls pass the caller's return address to the callee, so there is nothing to do
- `unwind` — I _think_ this is applicable too, although it's a bit confusing
- `call_source` — `become` forbids operators and is not created as a lowering of something else; tail calls always come from HIR (at least for now)

It might be helpful to read the interpreter implementation to understand what `TailCall` means exactly, although I've tried documenting it too.

-----

There are a few `FIXME`-questions still left, ideally we'd be able to answer them during review ':)

-----

r? `@oli-obk`
cc `@scottmcm` `@DrMeepster` `@JakobDegen`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 12, 2024
… r=workingjubilee

Revert accidental comment deletion

This reverts an accidental comment deletion made in rust-lang#113128

See also rust-lang#113128 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
Rollup merge of rust-lang#127625 - SkiFire13:revert-comment-deletion, r=workingjubilee

Revert accidental comment deletion

This reverts an accidental comment deletion made in rust-lang#113128

See also rust-lang#113128 (comment)
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 13, 2024
…, r=oli-obk,RalfJung

Support tail calls in mir via `TerminatorKind::TailCall`

This is one of the interesting bits in tail call implementation — MIR support.

This adds a new `TerminatorKind` which represents a tail call:
```rust
    TailCall {
        func: Operand<'tcx>,
        args: Vec<Operand<'tcx>>,
        fn_span: Span,
    },
```

*Structurally* this is very similar to a normal `Call` but is missing a few fields:
- `destination` — tail calls don't write to destination, instead they pass caller's destination to the callee (such that eventual `return` will write to the caller of the function that used tail call)
- `target` — similarly to `destination` tail calls pass the caller's return address to the callee, so there is nothing to do
- `unwind` — I _think_ this is applicable too, although it's a bit confusing
- `call_source` — `become` forbids operators and is not created as a lowering of something else; tail calls always come from HIR (at least for now)

It might be helpful to read the interpreter implementation to understand what `TailCall` means exactly, although I've tried documenting it too.

-----

There are a few `FIXME`-questions still left, ideally we'd be able to answer them during review ':)

-----

r? `@oli-obk`
cc `@scottmcm` `@DrMeepster` `@JakobDegen`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-explicit_tail_calls `#![feature(explicit_tail_calls)]` 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.