Remove improper use of each() in B::walksymtable and fix ext/B/t/xref.t
authorYves Orton <demerphq@gmail.com>
Wed, 21 Nov 2012 22:45:24 +0000 (23:45 +0100)
committerYves Orton <demerphq@gmail.com>
Wed, 21 Nov 2012 23:11:20 +0000 (00:11 +0100)
Improper use of each() in walksymtable() makes behavior undefined.

As of the hash randomization patch ext/B/t/xref.t would occasionally
fail. A closer inspection revealed that the output that is parsed
in t/xref.t would vary greatly from run to run, with bizarre double
entries and sometimes missing packages, etc. So in 100 runs we would see
something like 25 of one variant, and then 25 of another, and then about
50 singletons. The notable difference between the two large groups is
that one was missing entire modules worth of output, the other appeared
to be correct. The singletons tended to differ from one of the other
two by a line or two.

Changing the each() to a keys() appears to fix this bug, making the
output consistent every time. My theory is that it is possible for the
symbol table logic to recurse and enter the same package twice, which
would result in its each iterator changing state but I have not verfied
this.  In order to make sure that the traversal order is deterministic
I decided to use sort(keys()) (In other words the sort() is there to
guarantee a given output in the future, not to fix this bug).

Annoyingly many times the test would pass even though the output it was
parsing was grossly wrong. I think we need to figure out a better way to
test this module. We should probably check for the presence of various
packages, like the B package itsef, Errno.pm etc.

I cannot explain yet why this bug was sensitive to build options.
My normal build options seemed to not have any issue, wheras others did.

Apparently ok (with or without -Dusethreads):
    git clean -dfX; ./Configure -Doptimize=-g -d -Dusedevel -Dusethreads
        -Dprefix=~/bleadperl -Dcc=ccache\ gcc -Dld=gcc -DDEBUGGING

Regular fails:
    git clean -dfx; ./Configure -des -Dusedevel -Dprefix=~/bleadperl
        -Dcc=ccache\ gcc -Dld=gcc

My tests used the following one liners:

    for file in test{1..100}; do ./perl -Ilib ext/B/t/xref.t > $file 2>&1; done;
    md5sum test* | sort | uniq -c -w32 | sort -n

ext/B/B.pm
ext/B/t/xref.t

index 6e11611..b15b80e 100644 (file)
@@ -15,7 +15,7 @@ require Exporter;
 # walkoptree comes from B.xs
 
 BEGIN {
-    $B::VERSION = '1.40';
+    $B::VERSION = '1.41';
     @B::EXPORT_OK = ();
 
     # Our BOOT code needs $VERSION set, and will append to @EXPORT_OK.
@@ -250,7 +250,8 @@ sub walksymtable {
     my $fullname;
     no strict 'refs';
     $prefix = '' unless defined $prefix;
-    while (($sym, $ref) = each %$symref) {
+    foreach my $sym ( sort keys %$symref ) {
+        $ref= $symref->{$sym};
         $fullname = "*main::".$prefix.$sym;
        if ($sym =~ /::$/) {
            $sym = $prefix . $sym;
index 5d9cb38..32a80e7 100644 (file)
@@ -23,7 +23,7 @@ open SAVEOUT, ">&STDOUT" or diag $!;
 close STDOUT;
 # line 100
 our $compilesub = B::Xref::compile("-o$file");
-ok( ref $compilesub eq 'CODE', "compile() returns a coderef ($compilesub)" );
+ok( ref $compilesub eq 'CODE', "compile() returns a coderef" );
 $compilesub->(); # Compile this test script
 close STDOUT;
 open STDOUT, ">&SAVEOUT" or diag $!;
@@ -34,6 +34,7 @@ my ($curfile, $cursub, $curpack) = ('') x 3;
 our %xreftable = ();
 open XREF, $file or die "# Can't open $file: $!\n";
 while (<XREF>) {
+    print STDERR $_ if $ENV{PERL_DEBUG};
     chomp;
     if (/^File (.*)/) {
        $curfile = $1;