Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.85k stars 527 forks source link

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

Closed leonerd closed 2 weeks ago

leonerd commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

The test failures I cited last night are now PASSing.

leonerd commented 2 weeks ago

@haarg cpan> test Moo passed OK with this

leonerd commented 2 weeks ago

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.

mauke commented 2 weeks ago

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.