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

OP_SUBSTR_LEFT: GH#22914 - multiple pointers to replacement OP #22918

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

richardleach
Copy link
Contributor

The recent initial commit for OP_SUBSTR_LEFT failed to account for
there being multiple paths from a non-trivial LENGTH to the ""
replacement CONST OP. This could result in the replacement SV
being erroneously pushed to the stack, causing pp_substr_left
to try to operate on the wrong SV.

This commit nulls out the replacement OP, so that even if it
is encountered, no erroneous SV is pushed. Contrary to the
comment in the original commit, this actually does not break
B::Deparse.

Thanks to @mauke for figuring this out and preparing a patch
before I'd even opened my browser.


  • This set of changes does not require a perldelta entry.

The recent initial commit for OP_SUBSTR_LEFT failed to account for
there being multiple paths from a non-trivial LENGTH to the ""
replacement CONST OP. This could result in the replacement SV
being erroneously pushed to the stack, causing `pp_substr_left`
to try to operate on the wrong SV.

This commit nulls out the replacement OP, so that even if it
is encountered, no erroneous SV is pushed. Contrary to the
comment in the original commit, this actually does not break
B::Deparse.

Thanks to @mauke for figuring this out and preparing a patch
before I'd even opened my browser.
@richardleach
Copy link
Contributor Author

I'll look over this with fresh eyes in the morning, seemed worth opening this PR now for overnight CI etc.

@jkeenan
Copy link
Contributor

jkeenan commented Jan 16, 2025

I created a local branch from this p.r., then built an unthreaded build on Linux, ran the test suite and attempted to install the 3 CPAN distros identified so far as failing. Results good.

[gh-22918-richardleach-hydahy-substr_left_null_repl-20250115] 2102 $ ./bin/perl -Ilib -MIO::Async -MSpreadsheet::WriteExcel -MRDF::Trine -E 'say q|hello world|;'
hello world

Considerations:

  1. I suspect there will be addition CPAN breakage reported against cdbed2a.
  2. I haven't examined the code or test suite changes.

@richardleach
Copy link
Contributor Author

I created a local branch from this p.r., then built an unthreaded build on Linux, ran the test suite and attempted to install the 3 CPAN distros identified so far as failing. Results good.

Thanks for the prompt testing, @jkeenan.

While we may want to improve on the commit contents (e.g. tests) down the line, the better handling of multiple LENGTH pointers is clearly desirable. In the interest of reducing the number of BBC tickets coming in ahead of the next point release, I'm going to merge now and will keep an eye on incoming new issues.

@richardleach richardleach merged commit b1397c4 into Perl:blead Jan 16, 2025
34 checks passed
@richardleach richardleach deleted the hydahy/substr_left_null_repl branch January 16, 2025 10:54
@leonerd
Copy link
Contributor

leonerd commented Jan 16, 2025

Happy to confirm that this also fixes a recent IO::Async breakage:
http://www.cpantesters.org/cpan/report/e21668de-d265-11ef-b171-a1011627361f

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

Successfully merging this pull request may close these issues.

3 participants