tail calls: fix copying non-scalar arguments to callee#150473
tail calls: fix copying non-scalar arguments to callee#150473rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
3604bf5 to
0d2a400
Compare
|
☔ The latest upstream changes (presumably #150565) made this pull request unmergeable. Please resolve the merge conflicts. |
0d2a400 to
1bccfe8
Compare
This comment has been minimized.
This comment has been minimized.
|
I looked into this some at #144855 (comment). In summary, I believe that a tail call that uses an argument that is Supporting |
1bccfe8 to
b3bdf0f
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks, I had not seen that issue! What a mess. Whatever rules we come up with should be portable to all targets... Anyway, that's mostly orthogonal to what we do in the interpreter. Fixing the order in which we copy arguments and deallocate the old locals still makes sense IMO. |
|
@WaffleLapkin just a friendly ping -- are you okay with reviewing this or should I find another reviewer? |
This comment has been minimized.
This comment has been minimized.
| help: ALLOC was deallocated here: | ||
| --> tests/fail/tail_calls/dangling-local-var.rs:LL:CC | ||
| | | ||
| LL | f(std::ptr::null()); | ||
| | ^^^^^^^^^^^^^^^^^^^ | ||
| LL | let _val = unsafe { *x }; | ||
| | ^^^^ |
There was a problem hiding this comment.
arguably neither the old nor the new diagnostic is correct...
There was a problem hiding this comment.
Fair, though the new one is really strange... good point.
The old one kind of makes sense -- doing a tail call deallocates the locals of the old stack frame. (Though it'd be better to point at the become.)
There was a problem hiding this comment.
Oh wait, it used to point into main? Yeah that's nonsense.^^
b3bdf0f to
ad5108e
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=WaffleLapkin |
…ffleLapkin tail calls: fix copying non-scalar arguments to callee Alternative to rust-lang#144933: when invoking a tail call with a non-scalar argument, we need to delay freeing the caller's local variables until after the callee is initialized, so that we can copy things from the caller to the callee. Fixes rust-lang#144820... but as the FIXMEs in the code show, it's not clear to me whether these are the right semantics. r? @WaffleLapkin
…ffleLapkin tail calls: fix copying non-scalar arguments to callee Alternative to rust-lang#144933: when invoking a tail call with a non-scalar argument, we need to delay freeing the caller's local variables until after the callee is initialized, so that we can copy things from the caller to the callee. Fixes rust-lang#144820... but as the FIXMEs in the code show, it's not clear to me whether these are the right semantics. r? @WaffleLapkin
…ffleLapkin tail calls: fix copying non-scalar arguments to callee Alternative to rust-lang#144933: when invoking a tail call with a non-scalar argument, we need to delay freeing the caller's local variables until after the callee is initialized, so that we can copy things from the caller to the callee. Fixes rust-lang#144820... but as the FIXMEs in the code show, it's not clear to me whether these are the right semantics. r? @WaffleLapkin
Rollup of 19 pull requests Successful merges: - #145399 (Unify wording of resolve error) - #150473 (tail calls: fix copying non-scalar arguments to callee) - #152637 (Add a note about elided lifetime) - #152657 (std: move `exit` out of PAL) - #152729 (compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness") - #152753 (remove the explicit error for old `rental` versions) - #152758 (Remove ShallowInitBox.) - #151530 (Fix invalid `mut T` suggestion for `&mut T` in missing lifetime error) - #152179 (Add documentation note about signed overflow direction) - #152474 (Implement opt-bisect-limit for MIR) - #152509 (tests/ui/test-attrs: add annotations for reference rules) - #152672 (std: use libc version of `_NSGetArgc`/`_NSGetArgv`) - #152711 (resolve: Disable an assert that no longer holds) - #152732 (add regression test for 147958) - #152745 (Fix ICE in `suggest_param_env_shadowing` with incompatible args) - #152749 (make `rustc_allow_const_fn_unstable` an actual `rustc_attrs` attribute) - #152756 (Miri: recursive validity: also recurse into Boxes) - #152770 (carryless_mul: mention the base) - #152778 (Update tracking issue number for final_associated_functions)
Rollup of 20 pull requests Successful merges: - #145399 (Unify wording of resolve error) - #150473 (tail calls: fix copying non-scalar arguments to callee) - #152637 (Add a note about elided lifetime) - #152729 (compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness") - #152751 (Rename dep node "fingerprints" to distinguish key and value hashes) - #152753 (remove the explicit error for old `rental` versions) - #152758 (Remove ShallowInitBox.) - #151530 (Fix invalid `mut T` suggestion for `&mut T` in missing lifetime error) - #152179 (Add documentation note about signed overflow direction) - #152474 (Implement opt-bisect-limit for MIR) - #152509 (tests/ui/test-attrs: add annotations for reference rules) - #152672 (std: use libc version of `_NSGetArgc`/`_NSGetArgv`) - #152711 (resolve: Disable an assert that no longer holds) - #152725 (Rework explanation of CLI lint level flags) - #152732 (add regression test for 147958) - #152745 (Fix ICE in `suggest_param_env_shadowing` with incompatible args) - #152749 (make `rustc_allow_const_fn_unstable` an actual `rustc_attrs` attribute) - #152756 (Miri: recursive validity: also recurse into Boxes) - #152770 (carryless_mul: mention the base) - #152778 (Update tracking issue number for final_associated_functions)
Rollup merge of #150473 - RalfJung:interpret-tail-call, r=WaffleLapkin tail calls: fix copying non-scalar arguments to callee Alternative to #144933: when invoking a tail call with a non-scalar argument, we need to delay freeing the caller's local variables until after the callee is initialized, so that we can copy things from the caller to the callee. Fixes #144820... but as the FIXMEs in the code show, it's not clear to me whether these are the right semantics. r? @WaffleLapkin
Alternative to #144933: when invoking a tail call with a non-scalar argument, we need to delay freeing the caller's local variables until after the callee is initialized, so that we can copy things from the caller to the callee.
Fixes #144820... but as the FIXMEs in the code show, it's not clear to me whether these are the right semantics.
r? @WaffleLapkin