Skip to content

Commit

Permalink
fixup to "avoid identical stack traces"
Browse files Browse the repository at this point in the history
GH #15109, #17567

My original fix for this issue, v5.31.6-141-gf2f32cd638
made a shallow copy of &PL_compiling. However, for non-default
warning bits, this made two COPs share the malloced() cop_warnings,
and bad things ensured. In particular this was flagged up in:

    GH #17567: "BBC: AYOUNG/OpenVZ-0.01.tar.gz"

The fix in this commit is to do a deep copy of the COP using
newSTATEOP().
  • Loading branch information
iabyn committed Mar 12, 2020
1 parent 89561f3 commit fb8188b
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 9 deletions.
1 change: 1 addition & 0 deletions MANIFEST
Original file line number Diff line number Diff line change
Expand Up @@ -5647,6 +5647,7 @@ t/lib/feature/switch Tests for enabling/disabling switch feature
t/lib/GH_15109/Apack.pm test Module for caller.t
t/lib/GH_15109/Bpack.pm test Module for caller.t
t/lib/GH_15109/Cpack.pm test Module for caller.t
t/lib/GH_15109/Foo.pm test Module for caller.t
t/lib/h2ph.h Test header file for h2ph
t/lib/h2ph.pht Generated output from h2ph.h by h2ph, for comparison
t/lib/locale/latin1 Part of locale.t in Latin 1
Expand Down
6 changes: 2 additions & 4 deletions op.c
Original file line number Diff line number Diff line change
Expand Up @@ -11598,10 +11598,8 @@ S_process_special_blocks(pTHX_ I32 floor, const char *const fullname,
* to PL_compiling, IN_PERL_COMPILETIME/IN_PERL_RUNTIME
* will give the wrong answer.
*/
Newx(PL_curcop, 1, COP);
StructCopy(&PL_compiling, PL_curcop, COP);
PL_curcop->op_slabbed = 0;
SAVEFREEPV(PL_curcop);
PL_curcop = (COP*)newSTATEOP(PL_compiling.op_flags, NULL, NULL);
CopLINE_set(PL_curcop, CopLINE(&PL_compiling));
}

PUSHSTACKi(PERLSI_REQUIRE);
Expand Down
9 changes: 9 additions & 0 deletions t/lib/GH_15109/Foo.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# for use by caller.t for GH #15109

package Foo;

sub import {
use warnings; # restore default warnings
() = caller(1); # this used to cause valgrind errors
}
1;
22 changes: 17 additions & 5 deletions t/op/caller.t
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ BEGIN {
chdir 't' if -d 't';
require './test.pl';
set_up_inc('../lib');
plan( tests => 109 ); # some tests are run in a BEGIN block
plan( tests => 111 ); # some tests are run in a BEGIN block
}

my @c;
Expand Down Expand Up @@ -349,6 +349,20 @@ do './op/caller.pl' or die $@;
like($Cpack::callers[$_], qr{GH_15109/Apack.pm:3}, "GH #15109 level $_") for 3..5;
like($Cpack::callers[$_], qr{\(eval \d+\):1}, "GH #15109 level $_") for 6..8;
like($Cpack::callers[$_], qr{caller\.t}, "GH #15109 level $_") for 9;

# GH #15109 followup - the original fix wasn't saving cop_warnings
# correctly and this code used to crash or fail valgrind

my $w = 0;
local $SIG{__WARN__} = sub { $w++ };
eval q{
use warnings;
no warnings 'numeric'; # ensure custom cop_warnings
use Foo; # this used to mess up warnings flags
BEGIN { my $x = "foo" + 1; } # potential "numeric" warning
};
is ($@, "", "GH #15109 - eval okay");
is ($w, 0, "GH #15109 - warnings restored");
}

{
Expand All @@ -357,11 +371,9 @@ do './op/caller.pl' or die $@;
my ($pkg, $file, $line) = caller;
::is $file, 'virtually/op/caller.t', "BEGIN block sees correct caller filename";
::is $line, 12345, "BEGIN block sees correct caller line";
TODO: {
local $::TODO = "BEGIN blocks have wrong caller package [perl #129239]";
::is $pkg, 'RT129239', "BEGIN block sees correct caller package";
}
::is $pkg, 'RT129239', "BEGIN block sees correct caller package";
#line 12345 "virtually/op/caller.t"
}

}

3 comments on commit fb8188b

@atoomic
Copy link
Member

Choose a reason for hiding this comment

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

This is generating some random issues during global destruction
example: Unbalanced string table refcount: (1) for "open_IN" during global destruction.
view https://github.com/atoomic/perl5/runs/504137544?check_suite_focus=true

We should consider reverting this commit and come back with a later commit
@iabyn are you fine if I revert it?

@atoomic
Copy link
Member

Choose a reason for hiding this comment

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

I've reverted the commit and going to tag the cases with it

@iabyn
Copy link
Contributor Author

@iabyn iabyn commented on fb8188b Mar 18, 2020 via email

Choose a reason for hiding this comment

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

Please sign in to comment.