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

Avoid unnecessary "changing use VERSION ..." message around string eval (fixes #22121) #22175

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Apr 25, 2024

We need to save the value of PL_prevailing_version at the time the eval op was compiled, so it can be put in place during the running code.

Ideally we'd do something more robust, like change the OP_ENTERVAL op class into UNOP_AUX, so that the aux vector can store additional information like the version number and perhaps the frozen hints hash.

In practice it is far too close to the 5.40 release to contemplate such a change now, so this is a less intrusive but hackier change to achieve the same aim.

This is in two commits, because the first commit fixes the bug but leaves an ugly $^H{"CORE/prevailing_version"} in the hints hash inside a string eval. Probably not a huge disaster but it's messy. The second commit tidies that up too, but is implemented in a bit of a weird way around not triggering PERL_MAGIC_hinthash while deleting the key. It's not great but gets the job done.

We can discuss whether we want to merge both commits, or just one the first one.

@jkeenan
Copy link
Contributor

jkeenan commented Apr 26, 2024

Once I saw the test failures in 3 files reported by CI, I built and tested your branch on both Linux and FreeBSD. Though the failures I got there were similar to what was reported by CI, there were some interesting differences in error output, most notably:

Ubuntu Linux 22.04 LTS

$ uname -mrs
Linux 6.5.0-28-generic x86_64

$ gcc --version
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

$ git describe;gitcurr
v5.39.9-74-g384b3bcd93
leonerd-fix-gh-22121-20240425

$ ./perl -Ilib -V:config_args
config_args='-des -Dusedevel -Duseithreads';

$ cd t;./perl harness re/pat_re_eval.t; cd -
free(): double free detected in tcache 2
re/pat_re_eval.t .. No subtests run 

Test Summary Report
-------------------
re/pat_re_eval.t (Wstat: 134 (Signal: ABRT, dumped core) Tests: 0 Failed: 0)
  Non-zero wait status: 134
  Parse errors: No plan found in TAP output
Files=1, Tests=0,  0 wallclock secs ( 0.00 usr  0.00 sys +  0.01 cusr  0.00 csys =  0.01 CPU)
Result: FAIL

FreeBSD-13

$ uname -mrs
FreeBSD 13.2-RELEASE-p1 amd64

$ clang --version
FreeBSD clang version 14.0.5 (https://github.com/llvm/llvm-project.git llvmorg-14.0.5-0-gc12386ae247c)
...

$ git describe; gitcurr
v5.39.9-74-g384b3bcd93
leonerd-fix-22121-20240425

$ ./perl -Ilib -V:config_args
config_args='-des -Dusedevel -Duseithreads -Doptimize=-O2 -pipe -fstack-protector -fno-strict-aliasing';

$ cd t;./perl harness re/pat_re_eval.t; cd -
Global symbol "$t" requires explicit package name (did you forget to declare "my $t"?) at re/pat_re_eval.t line 847.
BEGIN not safe after errors--compilation aborted at re/pat_re_eval.t line 886.
re/pat_re_eval.t .. Dubious, test returned 2 (wstat 512, 0x200)
No subtests run 

Test Summary Report
-------------------
re/pat_re_eval.t (Wstat: 512 (exited 2) Tests: 0 Failed: 0)
  Non-zero exit status: 2
  Parse errors: No plan found in TAP output
Files=1, Tests=0,  0 wallclock secs ( 0.02 usr  0.01 sys +  0.05 cusr  0.00 csys =  0.07 CPU)
Result: FAIL

Note the free(): double free detected in tcache 2 in the error output on Linux/gcc -- not present on FreeBSD/clang. But then on FreeBSD there's a warning that didn't appear on Linux: Global symbol "$t" requires explicit package name ...

@jkeenan
Copy link
Contributor

jkeenan commented Apr 26, 2024

The test failures I cited last night are now PASSing.

@leonerd
Copy link
Contributor Author

leonerd commented Apr 26, 2024

@haarg cpan> test Moo passed OK with this

@leonerd leonerd force-pushed the gh22121 branch 2 times, most recently from e9ebc5b to 6cede0e Compare April 26, 2024 16:55
@leonerd
Copy link
Contributor Author

leonerd commented Apr 26, 2024

Here's some commits that attempt to fix it. See PR description for a fuller detail.

I vote we merge both, but if the second commit seems weird I'm happy to take just the first, and we'll document the weirdness. Somewhere.

@leonerd leonerd marked this pull request as ready for review April 26, 2024 17:00
pp_ctl.c Outdated Show resolved Hide resolved
…al op

We need to save the value of PL_prevailing_version at the time the eval
op was compiled, so it can be put in place during the running code.

Ideally we'd do something more robust, like change the OP_ENTERVAL op
class into UNOP_AUX, so that the aux vector can store additional
information like the version number and perhaps the frozen hints hash.

In practice it is far too close to the 5.40 release to contemplate such
a change now, so this is a less intrusive but hackier change to achieve
the same aim.

See also
  Perl#22121
@mauke
Copy link
Contributor

mauke commented Apr 27, 2024

I don't completely understand the ramifications of the code change, but it looks plausible to me and fixes the problem in my tests. So I'd say merge it.

@leonerd leonerd merged commit abc5e0f into Perl:blead Apr 27, 2024
29 checks passed
@leonerd leonerd deleted the gh22121 branch April 27, 2024 14:44
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