Skip to content

Fix fn_sig_for_fn_abi and the coroutine transform for generators#118219

Merged
bors merged 3 commits into
rust-lang:masterfrom
bjorn3:fix_generator_fn_abi
Nov 24, 2023
Merged

Fix fn_sig_for_fn_abi and the coroutine transform for generators#118219
bors merged 3 commits into
rust-lang:masterfrom
bjorn3:fix_generator_fn_abi

Conversation

@bjorn3

@bjorn3 bjorn3 commented Nov 23, 2023

Copy link
Copy Markdown
Member

There were three issues previously:

  • The self argument was pinned, despite Iterator::next taking an unpinned mutable reference.
  • A resume argument was passed, despite Iterator::next not having one.
  • The return value was CoroutineState<Item, ()> rather than Option

While these things just so happened to work with the LLVM backend, cg_clif does much stricter checks when trying to assign a value to a place. In addition it can't handle the mismatch between the amount of arguments specified by the FnAbi and the FnSig.

@rustbot

rustbot commented Nov 23, 2023

Copy link
Copy Markdown
Collaborator

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 23, 2023
@rustbot

rustbot commented Nov 23, 2023

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@bjorn3

bjorn3 commented Nov 23, 2023

Copy link
Copy Markdown
Member Author

Fixes #116447 (comment)

@bjorn3 bjorn3 mentioned this pull request Nov 23, 2023
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Fix fn_sig_for_fn_abi and the coroutine transform for generators
b7bc8d5 
There were three issues previously:
* The self argument was pinned, despite Iterator::next taking an
  unpinned mutable reference.
* A resume argument was passed, despite Iterator::next not having one.
* The return value was CoroutineState<Item, ()> rather than Option<Item>

While these things just so happened to work with the LLVM backend,
cg_clif does much stricter checks when trying to assign a value to a
place. In addition it can't handle the mismatch between the amount of
arguments specified by the FnAbi and the FnSig.
@bjorn3 bjorn3 force-pushed the fix_generator_fn_abi branch from f4e4aa7 to b7bc8d5 Compare November 23, 2023 20:17
@rustbot rustbot added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label Nov 23, 2023
compiler-errors
compiler-errors approved these changes Nov 23, 2023
View reviewed changes

@compiler-errors compiler-errors left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

r=me with or without doing more exhaustive matching on coroutine kind

Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated
Comment thread compiler/rustc_ty_utils/src/abi.rs Outdated
@compiler-errors

compiler-errors commented Nov 23, 2023

Copy link
Copy Markdown
Contributor

Thanks @bjorn3 for catching this. I really appreciate that clif is a lot less forgiving with ABI -- you've already caught a few of these mismatched in the past.

@bjorn3

bjorn3 commented Nov 23, 2023

Copy link
Copy Markdown
Member Author

Added exhaustive matching and fixed the assertion on the resume type.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the fix_generator_fn_abi branch from ceb5f13 to f711dd8 Compare November 23, 2023 20:43
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the fix_generator_fn_abi branch from f711dd8 to b3f9fcb Compare November 23, 2023 21:25
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the fix_generator_fn_abi branch from b3f9fcb to 543e559 Compare November 23, 2023 21:32
@bjorn3

bjorn3 commented Nov 23, 2023

Copy link
Copy Markdown
Member Author

@bors r=compiler-errors

@bors

bors commented Nov 23, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit 543e559 has been approved by compiler-errors

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2023
@bors

bors commented Nov 24, 2023

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 543e559 with merge 1fd418f...

@bors

bors commented Nov 24, 2023

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 1fd418f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 24, 2023
@bors bors merged commit 1fd418f into rust-lang:master Nov 24, 2023
@rustbot rustbot added this to the 1.76.0 milestone Nov 24, 2023
@rust-timer

rust-timer commented Nov 24, 2023

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (1fd418f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 675.904s -> 675.686s (-0.03%)
Artifact size: 313.71 MiB -> 313.72 MiB (0.00%)

@bjorn3 bjorn3 deleted the fix_generator_fn_abi branch November 24, 2023 09:05
@dlight

dlight commented Jan 14, 2024

Copy link
Copy Markdown

While these things just so happened to work with the LLVM backend, cg_clif does much stricter checks when trying to assign a value to a place.

Here you did mention that despite the LLVM backend accepting it, it was triggering UB. Does this PR fixes this for LLVM as well?

@bjorn3

bjorn3 commented Jan 14, 2024

Copy link
Copy Markdown
Member Author

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@compiler-errors compiler-errors compiler-errors approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@compiler-errors compiler-errors

Labels

A-meta Area: Issues & PRs about the rust-lang/rust repository itself 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

Milestone

1.76.0

Development

Successfully merging this pull request may close these issues.

8 participants

@bjorn3 @rustbot @rust-log-analyzer @compiler-errors @bors @rust-timer @dlight @davidtwco