-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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
base: master
Are you sure you want to change the base?
Support tail calls in mir via TerminatorKind::TailCall
#113128
Conversation
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 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 |
This comment has been minimized.
This comment has been minimized.
574d9a5
to
7ba9e99
Compare
let target = self.cfg.start_new_block(); | ||
let terminator = TerminatorKind::Drop { | ||
target, | ||
// The caller will handle this if needed. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@@ -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 |
There was a problem hiding this comment.
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;
?
There was a problem hiding this comment.
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, .. } => { |
There was a problem hiding this comment.
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?
@@ -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) |
There was a problem hiding this comment.
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.
I feel this is a fast track to UB, unless we have MIR validations to ensure It might make more sense to have an |
@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 |
I think we need to guarantee at the tail call site, that no things with a destructor are still in scope. |
If we have We will need to have checks somewhere to prevent this from happening. |
@oli-obk this is trivially guaranteed by the drop order (drops are run before the tail call), or did I misunderstood your question? |
@nbdd0121 right, I see. When can we generate I'll try adding a mir validator check, but it will only allow us to catch problems, not prevent them. |
The unwindability is part of the function's ABI or codegen attr that is computed by 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();
} |
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 |
☔ The latest upstream changes (presumably #113260) made this pull request unmergeable. Please resolve the merge conflicts. |
// 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. |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 typeu8
and returns2
-- 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
:
rust/compiler/rustc_const_eval/src/interpret/terminator.rs
Lines 578 to 587 in af3f2f8
// 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 saysUnreachable
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened an issue: rust-lang/unsafe-code-guidelines#491.
&prev_frame.return_place, | ||
ret, | ||
unwind, | ||
)?; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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:
rust/src/tools/miri/src/concurrency/thread.rs
Line 186 in 8d361cb
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...
119cb0d
to
406bd6b
Compare
☔ The latest upstream changes (presumably #113569) made this pull request unmergeable. Please resolve the merge conflicts. |
@WaffleLapkin any updates on this? thanks |
@Dylan-DPC the labels are accurate: this is waiting on me resolving conflicts and addressing review comments. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #122140) made this pull request unmergeable. Please resolve the merge conflicts. |
487678d
to
9510f11
Compare
This comment was marked as resolved.
This comment was marked as resolved.
9510f11
to
02d3da2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
02d3da2
to
d548066
Compare
@RalfJung okay, I think I finally worked out a solution to all the interpreter+tailcalls problems (well, your last suggestion worked). It's not pretty (can't share much code between normal and tail calls due to subtle differences), but it does appear to work. Can you take a look? 😺 |
d548066
to
1075069
Compare
Some changes occurred in coverage instrumentation. cc @Zalathar |
☔ The latest upstream changes (presumably #123322) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry for the long delay... it's hard to find a large enough block of time to be able to dig into a PR like this that can't just be handled with a drive-by review. And yet I still don't really have time for more than a drive-by review and it's not going to become better for at least another month. :( On a first read through the interpreter diff, things largely looked very reasonable. I was not able to fully see through the maze of functions involved in stack pop, but honestly the best way for me to try to understand that would probably be to check out the branch and try to refactor it into fewer functions. ;) The main thing that's missing is tests. There should be a bunch of tail call tests for Miri to make sure all the corner cases work correctly -- including things like
Which code were you hoping to share that you can't currently share? |
/// Return type of [`InterpCx::pop_stack_frame`]. | ||
pub struct StackPop<'tcx, Prov: Provenance> { | ||
pub jump: StackPopJump, | ||
pub target: StackPopCleanup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's a "jump" and a "target"... what does that mean...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jump
is whatever the caller needs to execute a jump afterwards. target
is the popped stack frame's .return_to_block
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can read the types myself. :) But those sound like it's twice the same thing (obviously the caller needs to know the target to execute the jump). So there should be a comment here explaining how they interact.
// Note that its locals are gone already, but that's fine. | ||
let frame = | ||
self.stack_mut().pop().expect("tried to pop a stack frame, but there were none"); | ||
let frame = self.pop_stack_frame(unwinding)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable has type StackPop
... please don't call it frame
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Do you have ideas for the name? I see how frame
can be misleading, but stack_pop
is not very descriptive either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If in doubt I'd name it after the type it has. That's definitely better than frame
.^^ If that name is not very descriptive then maybe the type needs to be renamed as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't love the type's name too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stack_pop_action
/stack_pop_info
would work for me. 🤷 At least they are a lot better than frame
.
@@ -36,6 +36,9 @@ pub enum StackPopJump { | |||
/// Indicates that we should *not* jump to the return/unwind address, as the callback already | |||
/// took care of everything. | |||
NoJump, | |||
|
|||
/// Returned by [`InterpCx::pop_stack_frame`] when no cleanup should be done. | |||
NoCleanup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what does it mean if a machine hook returns this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. My answer would probably be "it shouldn't return this", but I have no idea on how to enforce this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a start, an assertion would be an option?
Yeah, I totally get how reviewing this may be difficult. Not sure how to make this better though... I'll look into writing/copying tests for miri. I'd guess most of the tests I will have to write new, because rustc's backends don't support tail calls yet and so the only tests I can write are "compile error" or "CTFE".
To be quite honest, this completely washed out of my cache. I think I may have come up with this sentence before writing the current version of the code, I remember it referring to |
I think it's fine to land, maybe do a pass over all these functions to ensure their doc comments are up-to-date and sufficiently clear -- the rest we can then clean up later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments I thought of while reading part way through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments
d5cadf6
to
6e5edac
Compare
This is one of the interesting bits in tail call implementation — MIR support.
This adds a new
TerminatorKind
which represents a tail call: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 eventualreturn
will write to the caller of the function that used tail call)target
— similarly todestination
tail calls pass the caller's return address to the callee, so there is nothing to dounwind
— I think this is applicable too, although it's a bit confusingcall_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