FETCH called twice

[p5pRT]

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

Searchable as RT76814$

[p5pRT]

From @Abigail

Created by @Abigail

A couple of my CPAN modules broke recently, due to FETCH, under certain
conditions, being called twice.

This is a reduced test case​:

  use 5.010;
  use strict;
  use warnings;

  my $count;

  sub TIESCALAR {bless []}
  sub FETCH {$count ++; 1}

  tie my $foo => "main";

  {no warnings 'void'; $foo eq 'foo';}

  say $count;

  __END__

This should print '1'. However, with blead, it prints 2.

git bisect tells commit 6f1401d is the
culprit. This is the commit entry​:

commit 6f1401d
Author​: David Mitchell <davem@​iabyn.com>
Date​: Fri May 21 14​:18​:21 2010 +0100

  make overload respect get magic

  In most places, ops checked their args for overload *before* doing
  mg_get(). This meant that, among other issues, tied vars that
  returned overloaded objects wouldn't trigger calling the
  overloaded method. (Actually, for tied and arrays and hashes, it
  still often would since mg_get gets called beforehand in rvalue
  context).

  This patch does the following​:

  Makes sure get magic is called first.
 
  Moves most of the overload code formerly included by macros at the
  start of each pp function into the separate helper functions
  Perl_try_amagic_bin, Perl_try_amagic_un, S_try_amagic_ftest,
  with 3 new wrapper macros​:
  tryAMAGICbin_MG, tryAMAGICun_MG, tryAMAGICftest_MG.
  This made the code 3800 bytes smaller.

  Makes sure that FETCH is not called multiple times. Much of this
  bit was helped by some earlier work from Father Chrysostomos.
 
  Added new functions and macros sv_inc_nomg(), sv_dec_nomg(),
  dPOPnv_nomg, dPOPXiirl_ul_nomg, dPOPTOPnnrl_nomg, dPOPTOPiirl_ul_nomg
  dPOPTOPiirl_nomg, SvIV_please_nomg, SvNV_nomg (again, some of
  these were based on Father Chrysostomos's work).
 
  Fixed the list version of the repeat operator (x)​: it now only
  calls overloaded methods for the scalar version​:
  (1,2,$overloaded) x 10
 
  The only thing I haven't checked/fixed yet is overloading the
  iterator operator, <>.

I'll write a couple of test cases for this and commit them.

Abigail

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.12.1:

Configured by abigail at Wed May 19 02:45:31 CEST 2010.

Summary of my perl5 (revision 5 version 12 subversion 1) configuration:
   
  Platform:
    osname=linux, osvers=2.6.11-1.1369_fc4smp, archname=i686-linux-64int-ld
    uname='linux almanda 2.6.11-1.1369_fc4smp #1 smp thu jun 2 23:08:39 edt 2005 i686 i686 i386 gnulinux '
    config_args='-des -Dusedevel -Uversiononly -Dmydomain=.abigail.be -Dcf_email=abigail@abigail.be -Dperladmin=abigail@abigail.be -Doptimize=-g -Dcc=gcc -Dprefix=/opt/perl -Dusemorebits'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=undef, uselongdouble=define
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-g',
    cppflags='-DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include'
    ccversion='', gccversion='4.0.2 20051125 (Red Hat 4.0.2-8)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='long double', nvsize=12, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.3.5.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.3.5'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -g -L/usr/local/lib'

Locally applied patches:
    


@INC for perl 5.12.1:
    /home/abigail/Perl
    /opt/perl/lib/site_perl/5.12.1/i686-linux-64int-ld
    /opt/perl/lib/site_perl/5.12.1
    /opt/perl/lib/5.12.1/i686-linux-64int-ld
    /opt/perl/lib/5.12.1
    /opt/perl/lib/site_perl/5.12.0/i686-linux-64int-ld
    /opt/perl/lib/site_perl/5.12.0
    /opt/perl/lib/site_perl/5.10.1
    /opt/perl/lib/site_perl/5.10.0
    /opt/perl/lib/site_perl/5.8.8
    /opt/perl/lib/site_perl
    .


Environment for perl 5.12.1:
    HOME=/home/abigail
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LC_CTYPE=en_US.utf8
    LC_MESSAGES=en_US.utf8
    LD_LIBRARY_PATH=/home/abigail/Lib:/usr/local/lib:/usr/lib:/lib:/usr/X11R6/lib
    LOGDIR (unset)
    PATH=/home/abigail/Bin:/opt/perl/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/games:/opt/git/bin:/home/abigail/Perl/Photos:/home/abigail/Perl/Bin:/opt/mysql/bin
    PERL5LIB=/home/abigail/Perl
    PERLDIR=/opt/perl
    PERL_BADLANG (unset)
    SHELL=/bin/bash

[p5pRT]

From @Abigail

On Tue, Jul 27, 2010 at 11​:42​:19PM +0200, Abigail wrote​:

A couple of my CPAN modules broke recently, due to FETCH, under certain
conditions, being called twice.

This is a reduced test case​:

use 5\.010;
use strict;
use warnings;

my $count;

sub TIESCALAR \{bless \[\]\}
sub FETCH \{$count \+\+; 1\}

tie my $foo => "main";

\{no warnings 'void'; $foo eq 'foo';\}

say $count;

\_\_END\_\_

This should print '1'. However, with blead, it prints 2.

I wrote some tests, and I found 13 cases in blead where FETCH is called
twice, where one would expect it to be called once​:

  $tied lt 1;
  $tied le 1;
  $tied eq 1;
  $tied ge 1;
  $tied gt 1;
  $tied ne 1;
  $tied cmp 1;

  !$tied

  $tied || 1;
  $tied or 1;

  m/$tied/
  s/$tied//;
  $tied =~ y///;

5.12.1 is slightly better, but there are still 9 cases where FETCH is
called twice instead of once​:

  $tied || 1;
  $tied or 1;

  abs $tied;
  int $tied;

  -t $tied;

  m/$tied/;
  s/$tied//;
  $tied =~ y///
 
  *$tied;

So, four cases are fixed in blead, five are still present, and eight new
cases have been introduced.

5.8.8 calls FETCH just once for m/$tied/, s/$tied// and -t $tied, *three*
times for -l $tied, and is otherwise identical to 5.12.1.

New tests are in t/op/tie_fetch_count.t.

Abigail

[p5pRT]

From @cpansprout

• The first patch fixes string comparison operators. It changes sv_eq, sv_cmp, sv_cmp_locale and sv_collxfrm to _flags forms, with macros under the old names for sv_eq and sv_collxfrm, but functions for sv_cmp* since pp_sort.c needs them.

• The second patch fixes ! by doing the same thing to sv_2bool and adding SvTRUE_nomg. It also corrects the docs that state incorrectly that SvTRUE does not handle magic.

• The third patch fixes m and s. It modifies pp_regcomp to avoid extra magic. It also corrects a bug in sv_catsv_flags, which would still call mg_get(ssv) even without the SV_GMAGIC flag set.

• The fourth patch fixes y. (This has caused double magick at least as far back as 5.6.2.)

• The || case is not a bug, as there are two separate operators operating on it in the test script. In

  $dummy = $x || $y

The || does mg_get($x). If it’s true it returns it and the = does mg_get($x). If $x is false, then $y is returned, so magic is called once on each of $x and $y. Similarly, && will seemingly call mg_get($x) twice if $x is false.

If you just write​:

  $x || $y

then magic is only called once on $x.

The fifth patch attached hereto corrects the test.

• The sixth attachment is not a patch, but a text file of entries to be copied and pasted into perldelta, patches for which go stale too quickly.

Let’s see whether I remember the attachments....

[p5pRT]

From @cpansprout

[1. string comparison](https://rt-archive.perl.org/perl5/Ticket/Attachment/720604/345754/1. string comparison)

[p5pRT]

From @cpansprout

[2. !](https://rt-archive.perl.org/perl5/Ticket/Attachment/720604/345756/2. !)

[p5pRT]

From @cpansprout

[3. m and s](https://rt-archive.perl.org/perl5/Ticket/Attachment/720604/345758/3. m and s)

[p5pRT]

From @cpansprout

[4. y](https://rt-archive.perl.org/perl5/Ticket/Attachment/720604/345760/4. y)

[p5pRT]

From @cpansprout

[5. ||](https://rt-archive.perl.org/perl5/Ticket/Attachment/720604/345762/5. ||)

[p5pRT]

From @cpansprout

================================
Changes to Existing Documenation

---

=head3 L<perlapi>

The documentation for the C<SvTRUE> macro was simply wrong in stating that
get-magic is not processed. It has been corrected.

=======
Testing

---

=item *

The new F<t/op/tie_fetch_count.t> script tests that operators only call
C<FETCH> once on each operand.

================
Internal Changes

---

=item New C<*_flags> functions for string comparison

The C<sv_cmp_flags>, C<sv_cmp_locale_flags>, C<sv_eq_flags> and
C<sv_collxfrm_flags> functions have been added. These are like their
non-_flags counterparts, but allow one to specify whether get-magic is
processed.

The C<sv_cmp>, C<sv_cmp_locale>, C<sv_eq> and C<sv_collxfrm> functions have
been replaced with wrappers around the new functions.

=item New C<sv_2bool_flags> function

This is like C<sv_2bool>, but it lets the calling code decide whether
get-magic is handled. C<sv_2bool> is now a macro that calls the new
function.

=item New C<SvTRUE_nomg> macro

This is just like C<SvTRUE>, except that it does not process magic. It uses
the new C<sv_2bool_flags> function.

=item C<sv_catsv_flags> fix

C<sv_catsv_flags> no longer calls C<mg_get> on its second argument (the
source string) if the flags passed to it do not include SV_GMAGIC. So it
now matches what the documentation says it does.

==================
Selected Bug Fixes

---

=item *

The C<y///> or C<tr///> operator now calls get-magic (e.g., the C<FETCH>
method of a tie) on its left-hand side just once, not twice [perl #76814].

=item *

=for comment
This bug was introduced in 5.13.1 or .2, so it should not be included in
perl5140delta.

String comparison (C<eq>, C<ne>, C<lt>, C<gt>, C<le>, C<ge> and
C<cmp>) and logical not (C<not> and C<!>) operators no longer call magic
(e.g., tie methods) twice on their operands [perl #76814].

=item *

=for comment
This bug was introduced in 5.13.1 or .2, so it should not be included in
perl5140delta.

When a tied (or other magic) variable is used as, or in, a regular
expression, it no longer has its C<FETCH> method called twice [perl
#76814].

[p5pRT]

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

[p5pRT]

From @Abigail

On Sun, Aug 01, 2010 at 12​:14​:52PM -0700, Father Chrysostomos wrote​:

Thank you for your patches.

• The first patch fixes string comparison operators. It changes sv_eq, sv_cmp, sv_cmp_locale and sv_collxfrm to _flags forms, with macros under the old names for sv_eq and sv_collxfrm, but functions for sv_cmp* since pp_sort.c needs them.

• The second patch fixes ! by doing the same thing to sv_2bool and adding SvTRUE_nomg. It also corrects the docs that state incorrectly that SvTRUE does not handle magic.

• The third patch fixes m and s. It modifies pp_regcomp to avoid extra magic. It also corrects a bug in sv_catsv_flags, which would still call mg_get(ssv) even without the SV_GMAGIC flag set.

• The fourth patch fixes y. (This has caused double magick at least as far back as 5.6.2.)

• The || case is not a bug, as there are two separate operators operating on it in the test script. In

$dummy = $x || $y

The || does mg_get($x). If it’s true it returns it and the = does mg_get($x). If $x is false, then $y is returned, so magic is called once on each of $x and $y. Similarly, && will seemingly call mg_get($x) twice if $x is false.

If you just write​:

$x || $y

then magic is only called once on $x.

I don't understand why it's not a bug. If I do

  $dummy = f () || g ();

people would consider it to be a bug if f () were to be called twice. Why
should $x be FETCHed twice then? In fact, IMO, if $x is tied, $x in
rvalue context should just be a shorthand for C<< (tied $x) -> FETCH >>.

The fifth patch attached hereto corrects the test.

• The sixth attachment is not a patch, but a text file of entries to be copied and pasted into perldelta, patches for which go stale too quickly.

Let’s see whether I remember the attachments....

Abigail

[p5pRT]

From @cpansprout

On Aug 19, 2010, at 8​:09 AM, Abigail wrote​:

On Sun, Aug 01, 2010 at 12​:14​:52PM -0700, Father Chrysostomos wrote​:

Thank you for your patches.

• The first patch fixes string comparison operators. It changes sv_eq, sv_cmp, sv_cmp_locale and sv_collxfrm to _flags forms, with macros under the old names for sv_eq and sv_collxfrm, but functions for sv_cmp* since pp_sort.c needs them.

• The second patch fixes ! by doing the same thing to sv_2bool and adding SvTRUE_nomg. It also corrects the docs that state incorrectly that SvTRUE does not handle magic.

• The third patch fixes m and s. It modifies pp_regcomp to avoid extra magic. It also corrects a bug in sv_catsv_flags, which would still call mg_get(ssv) even without the SV_GMAGIC flag set.

• The fourth patch fixes y. (This has caused double magick at least as far back as 5.6.2.)

• The || case is not a bug, as there are two separate operators operating on it in the test script. In

$dummy = $x || $y

The || does mg_get($x). If it’s true it returns it and the = does mg_get($x). If $x is false, then $y is returned, so magic is called once on each of $x and $y. Similarly, && will seemingly call mg_get($x) twice if $x is false.

If you just write​:

$x || $y

then magic is only called once on $x.

I don't understand why it's not a bug. If I do

$dummy = f () || g ();

people would consider it to be a bug if f () were to be called twice. Why
should $x be FETCHed twice then? In fact, IMO, if $x is tied, $x in
rvalue context should just be a shorthand for C<< (tied $x) -> FETCH >>.

(Except that (tied $x)->FETCH doesn’t allow $_[0] assignment to change what the variable is tied too. :-)

Is it really rvalue context? Consider the following examples​:

$dummy = \($x || $y);
$dummy = ${\($x || $y)};

Can you come up with a way to unify the concepts such that your test would work and my two examples would continue to work (oh, and ${\($a||$b)} = $c as well)?

In the mean time, is there any chance the first four patches could be applied?

The fifth patch attached hereto corrects the test.

• The sixth attachment is not a patch, but a text file of entries to be copied and pasted into perldelta, patches for which go stale too quickly.

Let’s see whether I remember the attachments....

Abigail

[p5pRT]

From @iabyn

On Sun, Aug 22, 2010 at 12​:16​:19PM -0700, Father Chrysostomos wrote​:

I don't understand why it's not a bug. If I do

$dummy = f () || g ();

people would consider it to be a bug if f () were to be called twice. Why
should $x be FETCHed twice then? In fact, IMO, if $x is tied, $x in
rvalue context should just be a shorthand for C<< (tied $x) -> FETCH >>.

(Except that (tied $x)->FETCH doesn’t allow $_[0] assignment to change what the variable is tied too. :-)

Is it really rvalue context? Consider the following examples​:

$dummy = \($x || $y);
$dummy = ${\($x || $y)};

Can you come up with a way to unify the concepts such that your test would work and my two examples would continue to work (oh, and ${\($a||$b)} = $c as well)?

In the mean time, is there any chance the first four patches could be applied?

Could you re-submit the patches,
a) with a binary backwards-compatibility entry for the old
now-replaced-with-macro functions in mathoms.c? See
0d7d409 for an example of this.

b) Could the patches be attached to the email in a format that 'git am'
understands, e.g. using git format-patch?

Thanks.

--
This email is confidential, and now that you have read it you are legally
obliged to shoot yourself. Or shoot a lawyer, if you prefer. If you have
received this email in error, place it in its original wrapping and return
for a full refund. By opening this email, you accept that Elvis lives.

[p5pRT]

From @cpansprout

On Aug 24, 2010, at 2​:38 PM, Dave Mitchell wrote​:

On Sun, Aug 22, 2010 at 12​:16​:19PM -0700, Father Chrysostomos wrote​:

I don't understand why it's not a bug. If I do

$dummy = f () || g ();

people would consider it to be a bug if f () were to be called twice. Why
should $x be FETCHed twice then? In fact, IMO, if $x is tied, $x in
rvalue context should just be a shorthand for C<< (tied $x) -> FETCH >>.

(Except that (tied $x)->FETCH doesn’t allow $_[0] assignment to change what the variable is tied too. :-)

Is it really rvalue context? Consider the following examples​:

$dummy = \($x || $y);
$dummy = ${\($x || $y)};

Can you come up with a way to unify the concepts such that your test would work and my two examples would continue to work (oh, and ${\($a||$b)} = $c as well)?

In the mean time, is there any chance the first four patches could be applied?

Could you re-submit the patches,
a) with a binary backwards-compatibility entry for the old
now-replaced-with-macro functions in mathoms.c? See
0d7d409 for an example of this.

If we do not maintain binary compatibility in blead, then why are we maintaining binary compatibility in blead? Or are these actually for code using the Perl_ forms?

b) Could the patches be attached to the email in a format that 'git am'
understands, e.g. using git format-patch?

Here they are.

Please note that patch number 5 (||) is still controversial, as I’m waiting for an answer from Abigail.

BTW, the same thing happens with *{}, which returns its operand if it is already a glob.

[p5pRT]

From @cpansprout

[1a. string comparison](https://rt-archive.perl.org/perl5/Ticket/Attachment/729764/350498/1a. string comparison)