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

SvTRUE changed behavior in 5.18 #12990

Closed
p5pRT opened this issue May 24, 2013 · 9 comments
Closed

SvTRUE changed behavior in 5.18 #12990

p5pRT opened this issue May 24, 2013 · 9 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented May 24, 2013

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

Searchable as RT118159$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 24, 2013

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!

Perl Info

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 25, 2013

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 25, 2013

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 25, 2013

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 26, 2013

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 26, 2013

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 26, 2013

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

@p5pRT p5pRT closed this May 26, 2013
@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 27, 2013

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."

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 15, 2013

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

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

Successfully merging a pull request may close this issue.

None yet
1 participant