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

Data::Dumper: Useqq interacts badly with overloading #7519

Closed
p5pRT opened this issue Oct 1, 2004 · 24 comments
Closed

Data::Dumper: Useqq interacts badly with overloading #7519

p5pRT opened this issue Oct 1, 2004 · 24 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 1, 2004

Migrated from rt.perl.org#31793 (status was 'resolved')

Searchable as RT31793$

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

From zefram@fysh.org

Created by zefram@fysh.org

I'm using Data​::Dumper to display Perl objects for debugging purposes.
I like to use Dumper's Useqq option, because it produces printable
output when given unprintable strings. However, when that option is
used, Dumper fails on an object blessed into a class that overload the
"cmp" operator. Test case​:

$ perl -MData​::Dumper -e 'package foo; use overload "cmp" => sub { 0 }; package main; $Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
Operation `""'​: no method found, argument in overloaded package foo at /usr/share/perl/5.8/overload.pm line 97.
$

If either the "use overload" statement or the setting of Useqq is removed
from the above, then it produces the expected output​:

$ perl -MData​::Dumper -e 'package foo; package main; $Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
$VAR1 = bless( {}, 'foo' );
$

Setting the Useperl option, which I tried in hope of working around this
problem, also serves to make it fail. With Useperl = 1, Dumper fails
on an object with this overload regardless of the setting of Useqq.

Perl Info

Flags:
    category=library
    severity=medium

Site configuration information for perl v5.8.3:

Configured by Debian Project at Sat Mar 27 17:07:14 EST 2004.

Summary of my perl5 (revision 5.0 version 8 subversion 3) configuration:
  Platform:
    osname=linux, osvers=2.4.25-ti1211, archname=i386-linux-thread-multi
    uname='linux kosh 2.4.25-ti1211 #1 thu feb 19 18:20:12 est 2004 i686 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i386-linux -Dprefix=/usr -Dprivlib=/usr/share/perl/5.8 -Darchlib=/usr/lib/perl/5.8 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.8.3 -Dsitearch=/usr/local/lib/perl/5.8.3 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dlibperl=libperl.so.5.8.3 -Dd_dosuid -des'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O3',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.3.3 (Debian 20040314)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=true, libperl=libperl.so.5.8.3
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.3:
    /etc/perl
    /usr/local/lib/perl/5.8.3
    /usr/local/share/perl/5.8.3
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.8
    /usr/share/perl/5.8
    /usr/local/lib/site_perl
    /usr/local/lib/perl/5.8.2
    /usr/local/share/perl/5.8.2
    .


Environment for perl v5.8.3:
    HOME=/root
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/sbin:/sbin:/usr/local/sbin:/usr/local/armourplate/bin:/usr/bin:/bin:/usr/local/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

From @demerphq

I'm using Data​::Dumper to display Perl objects for debugging purposes.
I like to use Dumper's Useqq option, because it produces printable
output when given unprintable strings. However, when that option is
used, Dumper fails on an object blessed into a class that overload the
"cmp" operator. Test case​:

$ perl -MData​::Dumper -e 'package foo; use overload "cmp" =>
sub { 0 }; package main; $Data​::Dumper​::Useqq = 1; print
Dumper(bless({}, "foo"))'
Operation `""'​: no method found, argument in overloaded
package foo at /usr/share/perl/5.8/overload.pm line 97.
$

You may want to try Data​::Dump​::Streamer instead. Especially if the primary
objective of using DD is to produce readable output and not as a
serialization store. IE if speed isn't an issue which given your use of
Useqq I would say that is the case here. Plus DDS is more accurate. (Even if
I say so myself.)

If either the "use overload" statement or the setting of
Useqq is removed
from the above, then it produces the expected output​:

$ perl -MData​::Dumper -e 'package foo; package main;
$Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
$VAR1 = bless( {}, 'foo' );
$

The XS does it right then, and the Pure Perl code doesn't. Now we know where
to look to patch DD.

Setting the Useperl option, which I tried in hope of working
around this
problem, also serves to make it fail. With Useperl = 1, Dumper fails
on an object with this overload regardless of the setting of Useqq.

This is to be expected as Useqq implies Useperl currently. Only the Pure
Perl implementation of Data​::Dumper supports the Useqq option.

HTH,
Yves

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

From rick@bort.ca

On Fri, Oct 01, 2004 at 03​:21​:20PM -0000, Zefram wrote​:

I'm using Data​::Dumper to display Perl objects for debugging purposes.
I like to use Dumper's Useqq option, because it produces printable
output when given unprintable strings. However, when that option is
used, Dumper fails on an object blessed into a class that overload the
"cmp" operator. Test case​:

$ perl -MData​::Dumper -e 'package foo; use overload "cmp" => sub { 0 }; package main; $Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
Operation `""'​: no method found, argument in overloaded package foo at /usr/share/perl/5.8/overload.pm line 97.

The problem is not the overloading of 'cmp', but that "foo" objects are
overloaded and don't supply a means to stringify. Such an incompletely
overloaded class will give you unexpected problems with lots of modules
so I don't think Data​::Dumper is broken. Either overload '""' or add
'fallback => 1' to the 'use overload' line.

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

From zefram@fysh.org

Rick Delaney via RT wrote​:

The problem is not the overloading of 'cmp', but that "foo" objects are
overloaded and don't supply a means to stringify.

The object has no reasonable stringification, but does have meaningful
comparison. What is the stringificiation operator being used for here?

                                              Such an incompletely

overloaded class will give you unexpected problems with lots of modules
so I don't think Data​::Dumper is broken. Either overload '""' or add
'fallback => 1' to the 'use overload' line.

Interesting. I've not come across this concept of "incompletely
overloaded" before. The documentation for overload suggests that
"fallback => 1" is a reasonable thing to do in these classes. If I
set fallback or supply a stringification function, then Dumper has no
problem any more. However, the stringified form of the object does not
appear in the output (which is correct​: I don't want the stringification
to be used), and in fact the stringification function isn't even called.
So why am I required to supply a stringification operator?

-zefram

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2004

From rick@bort.ca

On Fri, Oct 01, 2004 at 06​:49​:30PM +0100, Zefram wrote​:

Rick Delaney via RT wrote​:

The problem is not the overloading of 'cmp', but that "foo" objects are
overloaded and don't supply a means to stringify.

The object has no reasonable stringification, but does have meaningful
comparison. What is the stringificiation operator being used for here?

Don't know.

                                              Such an incompletely

overloaded class will give you unexpected problems with lots of modules
so I don't think Data​::Dumper is broken. Either overload '""' or add
'fallback => 1' to the 'use overload' line.

Interesting. I've not come across this concept of "incompletely
overloaded" before. The documentation for overload suggests that
"fallback => 1" is a reasonable thing to do in these classes. If I

That's what I usually do and I wish it was the default. It would also be
nice if the overload docs listed the minimal set of operations that are
required to prevent exceptions.

set fallback or supply a stringification function, then Dumper has no
problem any more. However, the stringified form of the object does not
appear in the output (which is correct​: I don't want the stringification
to be used), and in fact the stringification function isn't even called.
So why am I required to supply a stringification operator?

That's a good question. I guess the exception is being raised when it's
not necessary so there is a bug here after all. It would be helpful if
someone could reduce the test case.

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Oct 2, 2004

From rick@bort.ca

On Fri, Oct 01, 2004 at 03​:21​:20PM -0000, Zefram wrote​:

$ perl -MData​::Dumper -e 'package foo; use overload "cmp" => sub { 0 }; package main; $Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
Operation `""'​: no method found, argument in overloaded package foo at /usr/share/perl/5.8/overload.pm line 97.
$

That patch below fixes this. You can see that it used to try to call
the overloaded stringify only in the case where there is no such method!

--
Rick Delaney
rick@​bort.ca

Inline Patch
--- lib/overload.pm.orig	Sat Oct  2 00:28:13 2004
+++ lib/overload.pm	Sat Oct  2 00:28:18 2004
@@ -94,7 +94,7 @@
 }
 
 sub StrVal {
-  (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
+  (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
     (AddrRef(shift)) :
     "$_[0]";
 }
--- lib/overload.t.orig	Sat Oct  2 00:32:50 2004
+++ lib/overload.t	Sat Oct  2 00:46:10 2004
@@ -1097,8 +1097,6 @@
 test (!defined eval { $a->{b} });
 test ($@ =~ /zap/);
 
-test (overload::StrVal(qr/a/) =~ /^Regexp=SCALAR\(0x[0-9a-f]+\)$/);
-
 {
    package t229;
    use overload '='  => sub { 42 },
@@ -1139,6 +1137,23 @@
     sub numify { ${$_[0]} }
 }
 
+{
+    package perl31793;
+    use overload cmp => sub { 0 };
+    package main;
+    my $o  = bless [], 'perl31793';
+    my $no = bless [], 'no_overload';
+    test (overload::StrVal(\"scalar") =~ /^SCALAR\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal([])        =~ /^ARRAY\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal({})        =~ /^HASH\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal(sub{1})    =~ /^CODE\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal(\*GLOB)    =~ /^GLOB\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal(\$o)       =~ /^REF\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal(qr/a/)     =~ /^Regexp=SCALAR\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal($o)        =~ /^perl31793=ARRAY\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal($no)       =~ /^no_overload=ARRAY\(0x[0-9a-f]+\)$/);
+}
+
 # These are all check that overloaded values rather than reference addressess
 # are what is getting tested.
 my ($two, $one, $un, $deux) = map {new Numify $_} 2, 1, 1, 2;
@@ -1160,4 +1175,4 @@
     }
 }
 # Last test is:
-sub last {484}
+sub last {492}

@p5pRT
Copy link
Author

p5pRT commented Oct 2, 2004

From perl_dummy@bloodgate.com

-----BEGIN PGP SIGNED MESSAGE-----

moin,

Re​: [perl #31793] Data​::Dumper​: Useqq interacts badly with overloading

Rick wrote​:

On Fri, Oct 01, 2004 at 06​:49​:30PM +0100, Zefram wrote​:

Rick Delaney via RT wrote​:

The problem is not the overloading of 'cmp', but that "foo" objects are
overloaded and don't supply a means to stringify.

The object has no reasonable stringification, but does have meaningful
comparison. What is the stringificiation operator being used for here?

Don't know.

See below for some ideas.

                                              Such an incompletely

overloaded class will give you unexpected problems with lots of modules
so I don't think Data​::Dumper is broken. Either overload '""' or add
'fallback => 1' to the 'use overload' line.

Interesting. I've not come across this concept of "incompletely
overloaded" before. The documentation for overload suggests that
"fallback => 1" is a reasonable thing to do in these classes. If I

That's what I usually do and I wish it was the default. It would also be
nice if the overload docs listed the minimal set of operations that are
required to prevent exceptions.

I don't know if that is possible. It is always possible to make "incomplete"
overloading without fallback, and all will be ok unless someone uses an
operation that is undefined. (for instance you can have "<=>" and only use
$a < $b and never notice that stringification is missing)

set fallback or supply a stringification function, then Dumper has no
problem any more. However, the stringified form of the object does not
appear in the output (which is correct​: I don't want the stringification
to be used), and in fact the stringification function isn't even called.
So why am I required to supply a stringification operator?

That's a good question. I guess the exception is being raised when it's
not necessary so there is a bug here after all. It would be helpful if
someone could reduce the test case.

Instead of tracing to Dumper, I simple tried to add a '""' overload and die
in it to see from where it is called​:

###############################
#!/usr/bin/perl -w

package foo;

use overload

"cmp" => sub { 0 },
'""' => sub { require Carp; Carp​::croak("hm") };

package main;

use Data​::Dumper;

$Data​::Dumper​::Useqq = 1;

print Dumper(bless({}, "foo"));
###############################

However, that simple prints​:

$VAR1 = bless( {}, 'foo' );

on my system. Maybe this is a problem in overload.pm. overload.pm line 97
(where it fails) is StrVal() and this routine is called from​:

Data​::Dumper /usr/lib/perl5/5.8.3/i586-linux-thread-multi/Data/Dumper.pm 239

And *this* is​:

  ($realpack, $realtype, $id) =
  (overload​::StrVal($val) =~ /^(?​:(.*)\=)?([^=]*)\(([^\(]*)\)$/);

Somebody else might have an insight why overload.pm stringifies the object
in StrVal() and why this is necessary and if we can avoid it.

Best wishes,

Tels
- --
Signed on Sat Oct 2 11​:56​:07 2004 with key 0x93B84C15.
Visit my photo gallery at http​://bloodgate.com/photos/
PGP key on http​://bloodgate.com/tels.asc or per email.

"Naturally the parameter and boundary of their respective position and
magnitude are naturally determinable up to the limits of possible
measurement as stated by the general quantum hypothesis and Heisenberg's
uncertainty principle, but this indeterminacy in precise value is not a
consequence of quantum uncertainty. What this illustrates is that in
relation to indeterminacy in precise physical magnitude, the micro and
macroscopic are inextricably linked, both being a part of the same parcel,
rather than just a case of the former underlying and contributing to the
latter." -- Peter Lynd

-----BEGIN PGP SIGNATURE-----
Version​: GnuPG v1.2.4 (GNU/Linux)
Comment​: When cryptography is outlawed, bayl bhgynjf jvyy unir cevinpl.

iQEVAwUBQV586ncLPEOTuEwVAQHPdAf+PJD1/pYR0LLSBNcGxFI+I5j701S7NJNS
oQtPgFFqzn3w2PI1ARAEavzzPlpCcFWc1o7Q+6fMQpFf2qdI+XIIWH5uVq7HHeDh
Mxj2j5rFJPgmP2N3fIUKDsgJq6ezbTS2AQayDNEtI4SjELBUPIOqIpadc/KiPgEC
z2/xN77+hCSMvwrKWdCygloqX+QT0q60Aqb5CmOSPC3JKLNEEWwxmHB37S+EZJ08
ogQHdn4svFLCk7/0nq3+AaMiqeY51mD614GB5ifv9cnlH+aDpuFG4csi6jJ7yn9C
8WvlgzYqujood8qt4uRzWOw/x4FOnmJBYW5+APEnGv4jsyomnuGv8Q==
=5UlQ
-----END PGP SIGNATURE-----

@p5pRT
Copy link
Author

p5pRT commented Oct 4, 2004

From @rgs

Rick Delaney wrote​:

--- lib/overload.pm.orig Sat Oct 2 00​:28​:13 2004
+++ lib/overload.pm Sat Oct 2 00​:28​:18 2004

Thanks, applied to bleadperl as #23347.

@p5pRT
Copy link
Author

p5pRT commented Oct 23, 2004

From @nwc10

On Sat, Oct 02, 2004 at 01​:04​:49AM -0400, Rick Delaney wrote​:

On Fri, Oct 01, 2004 at 03​:21​:20PM -0000, Zefram wrote​:

$ perl -MData​::Dumper -e 'package foo; use overload "cmp" => sub { 0 }; package main; $Data​::Dumper​::Useqq = 1; print Dumper(bless({}, "foo"))'
Operation `""'​: no method found, argument in overloaded package foo at /usr/share/perl/5.8/overload.pm line 97.
$

That patch below fixes this. You can see that it used to try to call
the overloaded stringify only in the case where there is no such method!

--- lib/overload.pm.orig Sat Oct 2 00​:28​:13 2004
+++ lib/overload.pm Sat Oct 2 00​:28​:18 2004
@​@​ -94,7 +94,7 @​@​
}

sub StrVal {
- (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
(AddrRef(shift)) :
"$_[0]";
}

This seems to be doing something really horrible to URI, which fails
regression tests. URI's use of overloading seems to be simple​:

use overload ('""' => sub { ${$_[0]} },
  '==' => sub { overload​::StrVal($_[0]) eq
  overload​::StrVal($_[1])
  },
  fallback => 1,
  );

I can't work out why it's failing either - there's something really strange
going on. For example, if I single step in the debugger it passes.
Simplest example of the 3 tests that fail is t/rel.t

1..4
ok 1
Operation `eq'​: no method found,
  left argument in overloaded package URI​::http,
  right argument has no overloaded magic at t/rel.t line 13.

which fails on the last line of this script snippet​:

#!/usr/bin/perl -w

print "1..4\n";

use strict;
use URI;

my $uri = URI->new("http​://www.example.com/foo/bar/");

print "not " unless $uri->rel("http​://www.example.com/foo/bar/") eq "./";
print "ok 1\n";

print "not " unless $uri->rel("HTTP​://WWW.EXAMPLE.COM/foo/bar/") eq "./";

The error report is generated in Perl_amagic_call in gv.c. I don't understand
why the result from the first call shown to $uri->rel() generates no error,
but the second one does. If you cut&paste bits of this code around, you'll
see that all you need is one letter of different case in the hostname in the
argument to rel(). ie this fails​:

  print "not " unless $uri->rel("http​://www.examplE.com/foo/bar/") eq "./";

The change is also breaking Fotango work code, so I've backed it out of maint
on the assumption that as is it's going to break quite a lot of real-world
code. Until I understand what the causes of this are (and preferably a fix)
it can't go into a release, and right now I'm stuck and would like some help
in figuring out just what on Earth is going on here.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Oct 25, 2004

From rick@bort.ca

On Sat, Oct 23, 2004 at 08​:22​:28PM +0100, Nicholas Clark wrote​:

sub StrVal {
- (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
(AddrRef(shift)) :
"$_[0]";
}

This seems to be doing something really horrible to URI, which fails
regression tests. URI's use of overloading seems to be simple​:

use overload ('""' => sub { ${$_[0]} },
'==' => sub { overload​::StrVal($_[0]) eq
overload​::StrVal($_[1])
},
fallback => 1,
);

I can't work out why it's failing either - there's something really strange
going on. For example, if I single step in the debugger it passes.
Simplest example of the 3 tests that fail is t/rel.t

It passes in blead for me too but I think that's just a fluke. I think the
above patch might just be exposing problems with fallback inheritance.

This is what I think is happening (somewhat simplified)​:

URI​::http inherits from URI which is overloaded and has set

  $URI​::fallback = 1;# Really implemented as ${URI​::()"}

Perl initializes fallback in Gv_AMupdate by checking the glob returned
by gv_fetchmeth. So some time before the first test below is run,
gv_fetchmeth("URI​::http", "fallback") is run which returns
*URI​::fallback. The package is URI because there is no "fallback"
subroutine in URI​::http.

Using the returned glob, the actual fallback state can be checked​:

  *URI​::fallback{SCALAR};

which is set to 1. So far, so good.

But gv_fetchmeth caches methods it finds. Now when Gv_AMupdate is
called again for URI​::http, gv_fetchmeth returns *URI​::http​::fallback
for which $URI​::http​::fallback is undefined. Now there is no fallback
and we get the error below.

I'm not entirely sure what triggers Gv_AMupdate to be called and
recalculate the overload settings (loading modules?) But it is being
triggered by the $uri->rel("HTTP​://WWW.EXAMPLE.COM/foo/bar/") call
below.

I'm not really sure how this should be fixed. I see a few options​:

1. Don't cache fallback method.
2. Cache the found package along with the method.
3. Save the value of $fallback in the derived package.

Whatever we do, we should define fallback inheritance properly. The
overload docs have had this for a long time​:

  Note. "fallback" inheritance via @​ISA is not carved in
  stone yet, see "Inheritance and overloading".

plus a couple of other warnings.

Hope this helps.

1..4
ok 1
Operation `eq'​: no method found,
left argument in overloaded package URI​::http,
right argument has no overloaded magic at t/rel.t line 13.

which fails on the last line of this script snippet​:

#!/usr/bin/perl -w

print "1..4\n";

use strict;
use URI;

my $uri = URI->new("http​://www.example.com/foo/bar/");

print "not " unless $uri->rel("http​://www.example.com/foo/bar/") eq "./";
print "ok 1\n";

print "not " unless $uri->rel("HTTP​://WWW.EXAMPLE.COM/foo/bar/") eq "./";

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Oct 25, 2004

From rick@bort.ca

On Mon, Oct 25, 2004 at 12​:27​:07AM -0400, Rick Delaney wrote​:

On Sat, Oct 23, 2004 at 08​:22​:28PM +0100, Nicholas Clark wrote​:

sub StrVal {
- (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
(AddrRef(shift)) :
"$_[0]";
}
[snip]
But gv_fetchmeth caches methods it finds. Now when Gv_AMupdate is
called again for URI​::http, gv_fetchmeth returns *URI​::http​::fallback
for which $URI​::http​::fallback is undefined. Now there is no fallback
and we get the error below.

Looking further, it appears that Gv_AMupdate tries to avoid this caching
deliberately by passing -1 for level to gv_fetchmeth. I think the
reason it's getting cached is because overload​::Overloaded is
implemented as​:

  $package->can('()');

and C<can> calls gv_fetchmethod_autoload which in turn calls
gv_fetchmeth with a level of 0 (meaning cache). But if that is the
problem I think it should be possible to force this bug in blead, yet
I've had no luck at this.

Still, this patchlet does make the problem disappear in 5.8.5.

Inline Patch
--- gv.c.orig	Mon Oct 25 11:02:26 2004
+++ gv.c	Mon Oct 25 12:32:39 2004
@@ -441,7 +441,8 @@
 	ostash = stash;
     }
 
-    gv = gv_fetchmeth(stash, name, nend - name, 0);
+    gv = gv_fetchmeth(stash, name, nend - name, 
+            strEQ(name,"()") ? -1 : 0);
     if (!gv) {
 	if (strEQ(name,"import") || strEQ(name,"unimport"))
 	    gv = (GV*)&PL_sv_yes;


But I think the proper fix would be to make overload::Overloaded not call C\\. Patch after \.sig against blead does this\. I don't know if it would cause a noticeable performance hit\.

I'm not entirely sure what triggers Gv_AMupdate to be called

It's called by C<bless>. What is upsetting the values of
PL_sub_generation or PL_amagic_generation to make it recalc, I have no
idea.

--
Rick Delaney
rick@​bort.ca

Inline Patch
--- perl-current/lib/overload.pm	Mon Oct  4 12:08:29 2004
+++ perl-dev/lib/overload.pm	Mon Oct 25 15:06:27 2004
@@ -51,7 +51,11 @@
 sub Overloaded {
   my $package = shift;
   $package = ref $package if ref $package;
-  $package->can('()');
+  return 1 if defined &{ "$package\::()" };
+  for my $pkg (@{"$package\::ISA"}) {
+    return 1 if Overloaded($pkg);
+  }
+  return 0;
 }
 
 sub ov_method {

@p5pRT
Copy link
Author

p5pRT commented Oct 30, 2004

From @nwc10

On Mon, Oct 25, 2004 at 03​:21​:15PM -0400, Rick Delaney wrote​:

On Mon, Oct 25, 2004 at 12​:27​:07AM -0400, Rick Delaney wrote​:

On Sat, Oct 23, 2004 at 08​:22​:28PM +0100, Nicholas Clark wrote​:

sub StrVal {
- (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
(AddrRef(shift)) :
"$_[0]";
}
[snip]
But gv_fetchmeth caches methods it finds. Now when Gv_AMupdate is
called again for URI​::http, gv_fetchmeth returns *URI​::http​::fallback
for which $URI​::http​::fallback is undefined. Now there is no fallback
and we get the error below.

Looking further, it appears that Gv_AMupdate tries to avoid this caching
deliberately by passing -1 for level to gv_fetchmeth. I think the
reason it's getting cached is because overload​::Overloaded is
implemented as​:

$package\->can\('\(\)'\);

and C<can> calls gv_fetchmethod_autoload which in turn calls
gv_fetchmeth with a level of 0 (meaning cache). But if that is the
problem I think it should be possible to force this bug in blead, yet
I've had no luck at this.

Still, this patchlet does make the problem disappear in 5.8.5.

I have this feeling that we still don't fully understand the cause of the
problem. I'd much prefer a suite of regression tests (presumably mostly
TODOs) that nail down how we want things to behave, prior to altering the
implementation, and potentially causing other emergent behaviour to change.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Nov 1, 2004

From rick@bort.ca

On Sat, Oct 30, 2004 at 04​:59​:05PM +0100, Nicholas Clark wrote​:

I have this feeling that we still don't fully understand the cause of the
problem. I'd much prefer a suite of regression tests (presumably mostly
TODOs) that nail down how we want things to behave, prior to altering the
implementation, and potentially causing other emergent behaviour to change.

I agree, and might even write the tests if someone could describe how
they want things to behave.

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Nov 1, 2004

From @rgs

Rick Delaney wrote​:

Looking further, it appears that Gv_AMupdate tries to avoid this caching
deliberately by passing -1 for level to gv_fetchmeth. I think the
reason it's getting cached is because overload​::Overloaded is
implemented as​:

$package\->can\('\(\)'\);

and C<can> calls gv_fetchmethod_autoload which in turn calls
gv_fetchmeth with a level of 0 (meaning cache). But if that is the
problem I think it should be possible to force this bug in blead, yet
I've had no luck at this.

Maybe what we need is a version of can() that doesn't try to autoload ?
A kind of can_light. (If I remember correctly other bugs, that can be
useful somewhere else.)

@p5pRT
Copy link
Author

p5pRT commented Nov 1, 2004

From rick@bort.ca

On Mon, Nov 01, 2004 at 10​:09​:11PM +0100, Rafael Garcia-Suarez wrote​:

Maybe what we need is a version of can() that doesn't try to autoload ?
A kind of can_light. (If I remember correctly other bugs, that can be
useful somewhere else.)

Can we get by with just adding an extra arg to can()? Better make it a
hashref so we can freely add options later. We'd need at least two​:
"autoload" and "cache" (which both default to true) since autoload=>0
would not be sufficient for this bug. We might need more; it's hard to
tell looking at the code.

Why do we have two separate method fetchers (gv_fetchmeth and
gv_fetchmethod) anyway? Could we harmonize them like was done recently
for hash fetching?

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2004

From @rgs

Rick Delaney wrote​:

On Mon, Nov 01, 2004 at 10​:09​:11PM +0100, Rafael Garcia-Suarez wrote​:

Maybe what we need is a version of can() that doesn't try to autoload ?
A kind of can_light. (If I remember correctly other bugs, that can be
useful somewhere else.)

Can we get by with just adding an extra arg to can()? Better make it a
hashref so we can freely add options later. We'd need at least two​:
"autoload" and "cache" (which both default to true) since autoload=>0
would not be sufficient for this bug. We might need more; it's hard to
tell looking at the code.

Why do we have two separate method fetchers (gv_fetchmeth and
gv_fetchmethod) anyway? Could we harmonize them like was done recently
for hash fetching?

It appears that overload already provides a mycan function that does
this.

The more I think about it, the more I come to the conclusion that
the original patch was wrong. I hereby suggest this : (this makes URI's
test suite pass as well (and this indicates we need more tests for
overload))

Index​: lib/overload.pm

--- lib/overload.pm (revision 4009)
+++ lib/overload.pm (working copy)
@​@​ -94,7 +94,7 @​@​
}

sub StrVal {
- (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
  (AddrRef(shift)) :
  "$_[0]";
}
Index​: lib/overload.t

--- lib/overload.t (revision 4009)
+++ lib/overload.t (working copy)
@​@​ -1139,7 +1139,7 @​@​

{
  package perl31793;
- use overload cmp => sub { 0 };
+ use overload cmp => sub { 0 }, fallback => 1;
  package main;
  my $o = bless [], 'perl31793';
  my $no = bless [], 'no_overload';

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2004

From @rgs

Rafael Garcia-Suarez wrote​:

The more I think about it, the more I come to the conclusion that
the original patch was wrong. I hereby suggest this : (this makes URI's
test suite pass as well (and this indicates we need more tests for
overload))

I've applied this patch as #23469. The problem coming from Data​::Dumper,
a patch for Data​::Dumper is in the works.

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2004

From @rgs

Zefram wrote​:

So why am I required to supply a stringification operator?

Resuming the situation, the following program fails with the current
Data​::Dumper :

  use Data​::Dumper;package foo;use overload cmp=>sub{0};package main;
  $Data​::Dumper​::Useperl=1;print Dumper(bless({},"foo"));

(as you see it's not the fault of Useqq -- Useqq only selects the
pure-perl implementation over the XS one. So Useperl = 1 is sufficient
to trigger the bug)

This happens because Data​::Dumper uses the stringified form of the
reference to get its address. The following patch to Data​::Dumper uses
Scalar​::Util​::refaddr() instead, and thus fixes this bug.

Drawbacks​: it uses Scalar​::Util :) (which is in the core now) (and it
uses our(), bad me. Data​::Dumper 1.131 is not supposed to work on perl <
5.6.0, though)

So, Ilya, if you could integrate something like this in the next
Data​::Dumper, that would be helpful. I note also that changes 23244,
23369 and 23383 to Data​::Dumper in bleadperl are not integrated into
Data​::Dumper 1.121.

Index​: ext/Data/Dumper/Dumper.pm

--- ext/Data/Dumper/Dumper.pm (revision 4009)
+++ ext/Data/Dumper/Dumper.pm (working copy)
@​@​ -92,6 +92,18 @​@​
  return bless($s, $c);
}

+sub init_refaddr_format {
+ require Config;
+ my $f = $Config​::Config{uvxformat};
+ $f =~ tr/"//d;
+ our $refaddr_format = "0x%" . $f;
+}
+
+sub format_refaddr {
+ require Scalar​::Util;
+ sprintf our $refaddr_format, Scalar​::Util​::refaddr(shift);
+}
+
#
# add-to or query the table of already seen references
#
@​@​ -101,7 +113,7 @​@​
  my($k, $v, $id);
  while (($k, $v) = each %$g) {
  if (defined $v and ref $v) {
- ($id) = (overload​::StrVal($v) =~ /\((.*)\)$/);
+ $id = format_refaddr($v);
  if ($k =~ /^[*](.*)$/) {
  $k = (ref $v eq 'ARRAY') ? ( "\\\@​" . $1 ) :
  (ref $v eq 'HASH') ? ( "\\\%" . $1 ) :
@​@​ -171,6 +183,7 @​@​
  my(@​out, $val, $name);
  my($i) = 0;
  local(@​post);
+ init_refaddr_format();

  $s = $s->new(@​_) unless ref $s;

@​@​ -236,8 +249,10 @​@​
  $val->$freezer() if UNIVERSAL​::can($val, $freezer);
  }

- ($realpack, $realtype, $id) =
- (overload​::StrVal($val) =~ /^(?​:(.*)\=)?([^=]*)\(([^\(]*)\)$/);
+ require Scalar​::Util;
+ $realpack = Scalar​::Util​::blessed($val);
+ $realtype = $realpack ? Scalar​::Util​::reftype($val) : ref $val;
+ $id = format_refaddr($val);

  # if it has a name, we need to either look it up, or keep a tab
  # on it so we know when we hit it later
@​@​ -406,7 +421,7 @​@​
  my $ref = \$_[1];
  # first, catalog the scalar
  if ($name ne '') {
- ($id) = ("$ref" =~ /\(([^\(]*)\)$/);
+ $id = format_refaddr($ref);
  if (exists $s->{seen}{$id}) {
  if ($s->{seen}{$id}[2]) {
  $out = $s->{seen}{$id}[0];

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2004

From rick@bort.ca

On Wed, Nov 03, 2004 at 12​:20​:12PM +0100, Rafael Garcia-Suarez wrote​:

It appears that overload already provides a mycan function that does
this.

Oh, I didn't notice that. I think we should just use that then.

The more I think about it, the more I come to the conclusion that
the original patch was wrong. I hereby suggest this : (this makes URI's
test suite pass as well (and this indicates we need more tests for
overload))

Yes, see below.

Index​: lib/overload.pm

--- lib/overload.pm (revision 4009)
+++ lib/overload.pm (working copy)
@​@​ -94,7 +94,7 @​@​
}

sub StrVal {
- (ref $_[0] && Overloaded($_[0]) or ref($_[0]) eq 'Regexp') ?
+ (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
(AddrRef(shift)) :
"$_[0]";
}

I still think that this is wrong because it mean that

  overload​::StrVal($overloaded_object);

will croak if there is no stringify or fallback. I think it should work
on any overloaded (or not) object.

Index​: lib/overload.t

--- lib/overload.t (revision 4009)
+++ lib/overload.t (working copy)
@​@​ -1139,7 +1139,7 @​@​

{
package perl31793;
- use overload cmp => sub { 0 };
+ use overload cmp => sub { 0 }, fallback => 1;
package main;
my $o = bless [], 'perl31793';
my $no = bless [], 'no_overload';

You just deleted some tests! It's good to have tests for

  use overload cmp => sub { 0 }, fallback => 1;

but we still need tests for

  use overload cmp => sub { 0 };

.

--
Rick Delaney
rick@​bort.ca

@p5pRT
Copy link
Author

p5pRT commented Nov 3, 2004

From @rgs

Rick Delaney wrote​:

I still think that this is wrong because it mean that

overload&#8203;::StrVal\($overloaded\_object\);

will croak if there is no stringify or fallback. I think it should work
on any overloaded (or not) object.

Hmm, in this case I don't see why StrVal needs to check that its
argument overloads whatever at all. So I suggest the following patch
(not removing the AddrRef function for clarity).

Index​: lib/overload.pm

--- lib/overload.pm (revision 4014)
+++ lib/overload.pm (working copy)
@​@​ -93,11 +93,7 @​@​
  return sprintf("$class_prefix$type(0x%x)", $addr);
}

-sub StrVal {
- (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
- (AddrRef(shift)) :
- "$_[0]";
-}
+*StrVal = *AddrRef;

sub mycan { # Real can would leave stubs.
  my ($package, $meth) = @​_;
Index​: lib/overload.t

--- lib/overload.t (revision 4014)
+++ lib/overload.t (working copy)
@​@​ -1139,7 +1139,7 @​@​

{
  package perl31793;
- use overload cmp => sub { 0 }, fallback => 1;
+ use overload cmp => sub { 0 };
  package main;
  my $o = bless [], 'perl31793';
  my $no = bless [], 'no_overload';

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2004

From rick@bort.ca

On Wed, Nov 03, 2004 at 05​:25​:31PM +0100, Rafael Garcia-Suarez wrote​:

Hmm, in this case I don't see why StrVal needs to check that its
argument overloads whatever at all. So I suggest the following patch
(not removing the AddrRef function for clarity).

You're right, that's a better fix. But I think the addition of the test
case with fallback is still worth having; just look at how easy it is to
break things that it touches.

Replacement patch follows .sig.

--
Rick Delaney
rick@​bort.ca

Inline Patch
diff -pruN perl-current/lib/overload.pm perl-current-dev/lib/overload.pm
--- perl-current/lib/overload.pm	Wed Nov  3 08:20:38 2004
+++ perl-current-dev/lib/overload.pm	Wed Nov  3 19:12:13 2004
@@ -93,11 +93,7 @@ sub AddrRef {
 	return sprintf("$class_prefix$type(0x%x)", $addr);
 }
 
-sub StrVal {
-  (ref $_[0] && OverloadedStringify($_[0]) or ref($_[0]) eq 'Regexp') ?
-    (AddrRef(shift)) :
-    "$_[0]";
-}
+*StrVal = *AddrRef;
 
 sub mycan {				# Real can would leave stubs.
   my ($package, $meth) = @_;
diff -pruN perl-current/lib/overload.t perl-current-dev/lib/overload.t
--- perl-current/lib/overload.t	Wed Nov  3 08:20:38 2004
+++ perl-current-dev/lib/overload.t	Wed Nov  3 19:15:26 2004
@@ -1139,9 +1139,12 @@ test ($@ =~ /zap/);
 
 {
     package perl31793;
+    use overload cmp => sub { 0 };
+    package perl31793_fb;
     use overload cmp => sub { 0 }, fallback => 1;
     package main;
     my $o  = bless [], 'perl31793';
+    my $of = bless [], 'perl31793_fb';
     my $no = bless [], 'no_overload';
     test (overload::StrVal(\"scalar") =~ /^SCALAR\(0x[0-9a-f]+\)$/);
     test (overload::StrVal([])        =~ /^ARRAY\(0x[0-9a-f]+\)$/);
@@ -1151,6 +1154,7 @@ test ($@ =~ /zap/);
     test (overload::StrVal(\$o)       =~ /^REF\(0x[0-9a-f]+\)$/);
     test (overload::StrVal(qr/a/)     =~ /^Regexp=SCALAR\(0x[0-9a-f]+\)$/);
     test (overload::StrVal($o)        =~ /^perl31793=ARRAY\(0x[0-9a-f]+\)$/);
+    test (overload::StrVal($of)       =~ /^perl31793_fb=ARRAY\(0x[0-9a-f]+\)$/);
     test (overload::StrVal($no)       =~ /^no_overload=ARRAY\(0x[0-9a-f]+\)$/);
 }
 
@@ -1199,4 +1203,4 @@ foreach my $op (qw(<=> == != < <= > >=))
 
 
 # Last test is:
-sub last {496}
+sub last {497}

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2004

From @rgs

Rick Delaney wrote​:

You're right, that's a better fix. But I think the addition of the test
case with fallback is still worth having; just look at how easy it is to
break things that it touches.

Replacement patch follows .sig.

Thanks, applied as #23470 to bleadperl.

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2008

@smpeters - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant