From @wumpus

Created by @wumpus

In upgrading the blekko search engine backend to Perl 5.18.0, one of
our engineers noticed a change in the behavior in SvTRUE.

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.008008
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.016000
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.018000
true

Is this intended? We were surprised.

It became visible to us because some Graphics Magik functions return
an XS-generated return value which is typically 1 and '' (indicating 1
image extracted and no error), and our code then uses "if" to test for
an error.

Graphics Magick doesn't document this return value and does not test it.

We don't have an opinion about what behavior is correct here, but it
is surprising that something so fundamental would change.

Thanks!

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.18.0:

Configured by keith at Thu May 23 11:25:49 PDT 2013.

Summary of my perl5 (revision 5 version 18 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=2.6.18-308.16.1.el5, archname=x86_64-linux
    uname='linux robert-desk 2.6.18-308.16.1.el5 #1 smp tue oct 2 22:01:43 edt 2012 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Dprefix=/pm/bpm/live/sw/perl/2013-05-23_11.25.48 -Duserelocatableinc -Dprivlib=.../../perl -Darchlib=.../../perl -Dsitelib=.../../perl -Dsitearch=.../../perl -Dman3ext=3pm -Dpager=/usr/bin/less -isr -Accflags=-DPERL_USE_SAFE_PUTENV -Dstartperl=#!/usr/bin/env perl -DEBUGGING=-g -Dusesitecustomize'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-DPERL_USE_SAFE_PUTENV -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g',
    cppflags='-DPERL_USE_SAFE_PUTENV -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.1.2 20080704 (Red Hat 4.1.2-52)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.5.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.5'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.18.0:
    /home/keith/blekko/perl
    /home/keith/blekko/perl
    .


Environment for perl 5.18.0:
    HOME=/home/keith
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LC_CTYPE=en_US.UTF-8
    LD_LIBRARY_PATH=/home/keith/blekko/lib
    LOGDIR (unset)
    PATH=/home/keith/pkg/inst/bin:/home/keith/blekko/bin:/usr/lib64/qt-3.3/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @tonycoz

On Fri, May 24, 2013 at 01​:42​:52PM -0700, Greg Lindahl wrote​:

# New Ticket Created by Greg Lindahl
# Please include the string​: [perl #118159]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=118159 >

This is a bug report for perl from lindahl@​pbm.com,
generated with the help of perlbug 1.39 running under perl 5.18.0.

-----------------------------------------------------------------
[Please describe your issue here]

In upgrading the blekko search engine backend to Perl 5.18.0, one of
our engineers noticed a change in the behavior in SvTRUE.

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.008008
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.016000
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, ""; print $a ? "true" : "false";'
5.018000
true

Is this intended? We were surprised.

It became visible to us because some Graphics Magik functions return
an XS-generated return value which is typically 1 and '' (indicating 1
image extracted and no error), and our code then uses "if" to test for
an error.

Graphics Magick doesn't document this return value and does not test it.

We don't have an opinion about what behavior is correct here, but it
is surprising that something so fundamental would change.

This appears to have changed in 4bac9ae.

Previously SvTRUE was​:

# define SvTRUE(sv) ( \
  !sv \
  ? 0 \
  : SvPOK(sv) \
  ? (({XPV *nxpv = (XPV*)SvANY(sv); \
  nxpv && \
  (nxpv->xpv_cur > 1 || \
  (nxpv->xpv_cur && *(sv)->sv_u.svu_pv != '0')); }) \
  ? 1 \
  : 0) \
  : \
  SvIOK(sv) \
  ? SvIVX(sv) != 0 \
  : SvNOK(sv) \
  ? SvNVX(sv) != 0.0 \
  : sv_2bool(sv) )

which checks *only* the PV part of the scalar if the scalar is flagged
as a PV.

The new version, largely in SvTRUE_common()​:

#define SvTRUE_common(sv,fallback) ( \
  !SvOK(sv) \
  ? 0 \
  : (SvFLAGS(sv) & (SVf_POK|SVf_IOK|SVf_NOK)) \
  ? ( (SvPOK(sv) && SvPVXtrue(sv)) \
  || (SvIOK(sv) && SvIVX(sv) != 0) \
  || (SvNOK(sv) && SvNVX(sv) != 0.0)) \
  : (fallback))

checks each part of the SV until it finds a true part (if any).

Tony

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

From @cpansprout

On Fri May 24 21​:06​:07 2013, tonyc wrote​:

On Fri, May 24, 2013 at 01​:42​:52PM -0700, Greg Lindahl wrote​:

# New Ticket Created by Greg Lindahl
# Please include the string​: [perl #118159]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=118159 >

This is a bug report for perl from lindahl@​pbm.com,
generated with the help of perlbug 1.39 running under perl 5.18.0.

-----------------------------------------------------------------
[Please describe your issue here]

In upgrading the blekko search engine backend to Perl 5.18.0, one of
our engineers noticed a change in the behavior in SvTRUE.

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, "";
print $a ? "true" : "false";'
5.008008
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, "";
print $a ? "true" : "false";'
5.016000
false

$ perl -MScalar​::Util=dualvar -le 'print $]; $a = dualvar 1, "";
print $a ? "true" : "false";'
5.018000
true

Is this intended? We were surprised.

It became visible to us because some Graphics Magik functions return
an XS-generated return value which is typically 1 and '' (indicating
1
image extracted and no error), and our code then uses "if" to test
for
an error.

Graphics Magick doesn't document this return value and does not test
it.

We don't have an opinion about what behavior is correct here, but it
is surprising that something so fundamental would change.

This appears to have changed in 4bac9ae.

Previously SvTRUE was​:

# define SvTRUE(sv) ( \
!sv
\
? 0
\
: SvPOK(sv) \
? (({XPV *nxpv = (XPV*)SvANY(sv); \
nxpv && \
(nxpv->xpv_cur > 1 || \
(nxpv->xpv_cur && *(sv)->sv_u.svu_pv != '0')); }) \
? 1 \
: 0) \
: \
SvIOK(sv) \
? SvIVX(sv) != 0 \
: SvNOK(sv) \
? SvNVX(sv) != 0.0 \
: sv_2bool(sv) )

which checks *only* the PV part of the scalar if the scalar is flagged
as a PV.

The new version, largely in SvTRUE_common()​:

#define SvTRUE_common(sv,fallback) ( \
!SvOK(sv) \
? 0 \
: (SvFLAGS(sv) & (SVf_POK|SVf_IOK|SVf_NOK)) \
? ( (SvPOK(sv) && SvPVXtrue(sv)) \
|| (SvIOK(sv) && SvIVX(sv) != 0) \
|| (SvNOK(sv) && SvNVX(sv) != 0.0)) \
: (fallback))

checks each part of the SV until it finds a true part (if any).

Tony

I can’t believe I missed that when reviewing the patch in question.
Traditionally, truth was based solely on stringification. The
examination of the SvIVX and SvNVX slots was for those cases where there
was no string already and it could be deduced from IVX or NVX whether it
would stringify as "0" or no (bugs with -0 aside).

So this seems like a bug to me. Should we change it back?

--

Father Chrysostomos

From @rjbs

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-05-25T14​:20​:32]

So this seems like a bug to me. Should we change it back?

I think we should.

--
rjbs

From @cpansprout

On Sat May 25 18​:26​:31 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-05-
25T14​:20​:32]

So this seems like a bug to me. Should we change it back?

I think we should.

I have just done so in commit 762dbf2.

--

Father Chrysostomos

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

From @chipdude

On 5/26/2013 12​:20 AM, Father Chrysostomos via RT wrote​:

On Sat May 25 18​:26​:31 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-05-
25T14​:20​:32]

So this seems like a bug to me. Should we change it back?
I think we should.

I have just done so in commit 762dbf2.

Yup, it was inadvertent. "I'm the author of the bug, and I approve this
patch."

From @iabyn

On Sun, May 26, 2013 at 07​:06​:40PM -0700, Reverend Chip wrote​:

On 5/26/2013 12​:20 AM, Father Chrysostomos via RT wrote​:

On Sat May 25 18​:26​:31 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-05-
25T14​:20​:32]

So this seems like a bug to me. Should we change it back?
I think we should.

I have just done so in commit 762dbf2.

Yup, it was inadvertent. "I'm the author of the bug, and I approve this
patch."

Now cherry-picked into maint-5.18 as
55d1519.

--
All wight. I will give you one more chance. This time, I want to hear
no Wubens. No Weginalds. No Wudolf the wed-nosed weindeers.
  -- Life of Brian