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

ruby_3_2 backport for #8006 #8008

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Jun 30, 2023

This is a backported version of #8006
The patch is different because the two branches have diverged.

Original bug report: Shopify/yjit#313


YJIT: Fix autosplat miscomp for blocks with optionals

When passing an array as the sole argument to yield, and the yieldee takes more than 1 optional parameter, the array is expanded similar to *array splat calls. This is called "autosplat" in setup_parameters_complex().

Previously, YJIT did not detect this autosplat condition. It passed the array without expanding it, deviating from interpreter behavior. Detect this conditon and refuse to compile it.

When passing an array as the sole argument to `yield`, and the yieldee
takes more than 1 optional parameter, the array is expanded similar
to `*array` splat calls. This is called "autosplat" in
`setup_parameters_complex()`.

Previously, YJIT did not detect this autosplat condition. It passed the
array without expanding it, deviating from interpreter behavior.
Detect this conditon and refuse to compile it.
@matzbot matzbot requested a review from a team June 30, 2023 22:01
@XrXr XrXr removed the request for review from a team June 30, 2023 22:02
@maximecb
Copy link
Contributor

maximecb commented Jul 4, 2023

Everything passed except for something which seems to be a Cirrus CI issue, so I'm going to merge this.

@maximecb maximecb added this pull request to the merge queue Jul 4, 2023
Merged via the queue into ruby:ruby_3_2 with commit 2f603bc Jul 4, 2023
88 of 89 checks passed
@XrXr
Copy link
Member Author

XrXr commented Jul 4, 2023

Sorry, I forgot to mention that we're not supposed to merge changes to backport branches (though it's fine to make requests). The branch maintainer is given complete control over what lands. At least that's the policy last I checked.

What's done is done though, and no point worrying about it.

@XrXr XrXr deleted the yjit-32-autosplat-opts branch July 4, 2023 16:19
@hsbt
Copy link
Member

hsbt commented Jul 4, 2023

@maximecb Don't merge PR into ruby_3_2 branch. Stable branches can only be merged by @nagachika or @unak. And I can merge only test or CI fixes at ruby_3_2.

@nagachika
Copy link
Member

I have reviewed the changeset.

Also, if the changesets in the master branch cannot be cleanly applied, creating patches for the stable branches is always welcome. I will handle PRs when I am mentioned or assigned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants