This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
Deparse: avoid upgrading RV to GV in stash entries
authorDavid Mitchell <davem@iabyn.com>
Wed, 22 Nov 2017 09:51:24 +0000 (09:51 +0000)
committerDavid Mitchell <davem@iabyn.com>
Thu, 23 Nov 2017 08:52:16 +0000 (08:52 +0000)
As well as being undesirable in its own right, it was causing some subs
not to be deparsed.

In something like

    package Foo;
    sub f { ... }
    *g = \&f;

The stash entry $Foo::{f} is an RV pointing to the CV, while $Foo::{g} is
a GV whose CV slot points to the same CV.

That CV's CvNAME() will be 'f' and its CvSTASH() will point to %Foo::.

If Deparse attempts to process $Foo::{g} before $Foo::{f}, it will get a
GV and in that code path it does something like

    $cv = $gv->CV;
    next if $$gv != ${$cv->GV}; # Ignore imposters

The trouble is that $cv->GV calls (at the C level) CvGV(cv), which tries
to retrieve the GV stored in $Foo::{f}, and finding only an RV, upgrades
it to a GV.

This confuses Deparse, because it has already created objects for all the
stash's entries, so when it comes to process $Foo::{f}, it already
has a B::IV object for the RV (and so goes down the RV code path), but
further introspection of that object (such as flags) sees a GV,

Hence the 3 lines of code at the top of this text were being deparsed
without 'sub f {}' being emitted.

This has been a problem for a while, but only recently has the "RV->CV
instead of GV->CV" optimisation been applied outside of package main::,
and so become more noticeable.

lib/B/Deparse.pm
lib/B/Deparse.t

index 1107303..0cbb890 100644 (file)
@@ -656,7 +656,8 @@ sub stash_subs {
        if ($seen ||= {})->{
            $INC{"overload.pm"} ? overload::StrVal($stash) : $stash
           }++;
-    my %stash = svref_2object($stash)->ARRAY;
+    my $stashobj = svref_2object($stash);
+    my %stash = $stashobj->ARRAY;
     while (my ($key, $val) = each %stash) {
        my $flags = $val->FLAGS;
        if ($flags & SVf_ROK) {
@@ -697,7 +698,20 @@ sub stash_subs {
        } elsif (class($val) eq "GV") {
            if (class(my $cv = $val->CV) ne "SPECIAL") {
                next if $self->{'subs_done'}{$$val}++;
-               next if $$val != ${$cv->GV};   # Ignore imposters
+
+                # Ignore imposters (aliases etc)
+                my $name = $cv->NAME_HEK;
+                if(defined $name) {
+                    # avoid using $cv->GV here because if the $val GV is
+                    # an alias, CvGV() could upgrade the real stash entry
+                    # from an RV to a GV
+                    next unless $name eq $key;
+                    next unless $$stashobj == ${$cv->STASH};
+                }
+                else {
+                   next if $$val != ${$cv->GV};
+                }
+
                $self->todo($cv, 0);
            }
            if (class(my $cv = $val->FORM) ne "SPECIAL") {
index fe176e1..ca1bdb4 100644 (file)
@@ -13,7 +13,7 @@ BEGIN {
 use warnings;
 use strict;
 
-my $tests = 49; # not counting those in the __DATA__ section
+my $tests = 52; # not counting those in the __DATA__ section
 
 use B::Deparse;
 my $deparse = B::Deparse->new();
@@ -546,6 +546,22 @@ unlike runperl(stderr => 1, switches => [ '-MO=-qq,Deparse', $path, '-w' ],
        qr'Use of uninitialized value',
       'no warnings for undefined sub';
 
+is runperl(stderr => 1, switches => [ '-MO=-qq,Deparse', $path ],
+    prog => 'sub f { 1; } BEGIN { *g = \&f; }'),
+    "sub f {\n    1;\n}\nsub BEGIN {\n    *g = \\&f;\n}\n",
+    "sub glob alias shouldn't impede emitting original sub";
+
+is runperl(stderr => 1, switches => [ '-MO=-qq,Deparse', $path ],
+    prog => 'package Foo; sub f { 1; } BEGIN { *g = \&f; }'),
+    "package Foo;\nsub f {\n    1;\n}\nsub BEGIN {\n    *g = \\&f;\n}\n",
+    "sub glob alias outside main shouldn't impede emitting original sub";
+
+is runperl(stderr => 1, switches => [ '-MO=-qq,Deparse', $path ],
+    prog => 'package Foo; sub f { 1; } BEGIN { *Bar::f = \&f; }'),
+    "package Foo;\nsub f {\n    1;\n}\nsub BEGIN {\n    *Bar::f = \\&f;\n}\n",
+    "sub glob alias in separate package shouldn't impede emitting original sub";
+
+
 done_testing($tests);
 
 __DATA__