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

Blead Breaks CPAN: ADAMK/SQL-String-0.02.tar.gz #16393

Closed
p5pRT opened this issue Jan 29, 2018 · 9 comments
Closed

Blead Breaks CPAN: ADAMK/SQL-String-0.02.tar.gz #16393

p5pRT opened this issue Jan 29, 2018 · 9 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Jan 29, 2018

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

Searchable as RT132783$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 29, 2018

From @eserte

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.41 running under perl 5.27.8.


The test suite of SQL-String-0.02 started to fail with 5.27.7
(there's no bisect result yet)​:

...
# Looks like you planned 78 tests but ran 79.
t/02_main.t .....
Dubious, test returned 255 (wstat 65280, 0xff00)
All 78 subtests passed
...

On first glance it seems that we have now a warning more than
with 5.27.6.



Flags​:
  category=core
  severity=low


Site configuration information for perl 5.27.8​:

Configured by eserte at Sat Jan 20 09​:22​:10 CET 2018.

Summary of my perl5 (revision 5 version 27 subversion 8) configuration​:
 
  Platform​:
  osname=linux
  osvers=3.16.0-4-amd64
  archname=x86_64-linux
  uname='linux cabulja 3.16.0-4-amd64 #1 smp debian 3.16.51-3 (2017-12-13) x86_64 gnulinux '
  config_args='-ds -e -Dprefix=/opt/perl-5.27.8 -Dusedevel -Dusemallocwrap=no -Dcf_email=srezic@​cpan.org'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='4.9.2'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  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-strong -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'


@​INC for perl 5.27.8​:
  /opt/perl-5.27.8/lib/site_perl/5.27.8/x86_64-linux
  /opt/perl-5.27.8/lib/site_perl/5.27.8
  /opt/perl-5.27.8/lib/5.27.8/x86_64-linux
  /opt/perl-5.27.8/lib/5.27.8


Environment for perl 5.27.8​:
  HOME=/home/eserte
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/bin​:/usr/bin​:/bin​:/usr/local/sbin​:/usr/sbin​:/sbin​:/home/eserte/bin/linux-gnu​:/home/eserte/bin/sh​:/home/eserte/bin​:/home/eserte/bin/pistachio-perl/bin​:/usr/games​:/home/eserte/devel
  PERLDOC=-MPod​::Perldoc​::ToTextOverstrike
  PERL_BADLANG (unset)
  SHELL=/bin/zsh

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 29, 2018

From @jkeenan

On Mon, 29 Jan 2018 20​:25​:24 GMT, slaven@​rezic.de wrote​:

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.41 running under perl 5.27.8.

-----------------------------------------------------------------
The test suite of SQL-String-0.02 started to fail with 5.27.7
(there's no bisect result yet)​:

...
# Looks like you planned 78 tests but ran 79.
t/02_main.t .....
Dubious, test returned 255 (wstat 65280, 0xff00)
All 78 subtests passed
...

On first glance it seems that we have now a warning more than
with 5.27.6.

The additional warning is caught in a test related to overloading of the .= operator (approx test #47 in original).

#####
$ cat 132783-Sql-String-overload-dotequals.t
use strict;
BEGIN { $^W = 1; }
use Test​::More qw(no_plan); # tests => 78;
use SQL​::String ();
my $SQL = SQL​::String->new('foo = ?', 2);
{
  local $SIG{__WARN__} =
  sub { ok( 1, 'Caught warning during undef concat' ) };
  $SQL .= undef;
}
isa_ok( $SQL, 'SQL​::String' );
#####

Results under perl-5.26.0's prove​:

#####
ok 1 - Caught warning during undef concat
ok 2 - An object of class 'SQL​::String' isa 'SQL​::String'
1..2
ok
All tests successful.
Files=1, Tests=2, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.05 cusr 0.00 csys = 0.07 CPU)
Result​: PASS
#####

Results under blead's prove​:

#####
ok 1 - Caught warning during undef concat
ok 2 - Caught warning during undef concat
ok 3 - An object of class 'SQL​::String' isa 'SQL​::String'
1..3
ok
All tests successful.
Files=1, Tests=3, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.03 cusr 0.00 csys = 0.05 CPU)
Result​: PASS
#####

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 29, 2018

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 30, 2018

From @dur-randir

On Mon, 29 Jan 2018 12​:25​:24 -0800, slaven@​rezic.de wrote​:

The test suite of SQL-String-0.02 started to fail with 5.27.7
(there's no bisect result yet)​:

Bisect points to

d7e7503 is the first bad commit
commit d7e7503
Author​: David Mitchell <davem@​iabyn.com>
Date​: Tue Nov 28 09​:08​:09 2017 +0000

  $overloaded .= $x​: don't stringify $x

  RT #132385

  This is a variant of the ($ref . $overloaded) bug which was fixed with
  v5.27.5-195-gb3ab0375cb.

  Basically, when the overloaded concat method is called, it should pass
  $x as-is, rather than as "$x". This fixes PDL-2.018

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 30, 2018

From @iabyn

On Tue, Jan 30, 2018 at 01​:03​:23AM -0800, Sergey Aleynikov via RT wrote​:

On Mon, 29 Jan 2018 12​:25​:24 -0800, slaven@​rezic.de wrote​:

The test suite of SQL-String-0.02 started to fail with 5.27.7
(there's no bisect result yet)​:

Bisect points to

d7e7503 is the first bad commit
commit d7e7503
Author​: David Mitchell <davem@​iabyn.com>
Date​: Tue Nov 28 09​:08​:09 2017 +0000

$overloaded \.= $x&#8203;: don't stringify $x

RT \#132385

This is a variant of the \($ref \. $overloaded\) bug which was fixed with
v5\.27\.5\-195\-gb3ab0375cb\.

Basically\, when the overloaded concat method is called\, it should pass
$x as\-is\, rather than as "$x"\. This fixes PDL\-2\.018

The test case reduces to

  use warnings;
  package Foo { use overload '.=' => sub { return "foo"; }; }
  my $s = bless [], 'Foo';
  my $x;
  $s .= $x;

which used to be silent, but now emits​:

  Use of uninitialized value $x in concatenation (.) or string

I'll look into this further when I can find some time.

--
No man treats a motor car as foolishly as he treats another human being.
When the car will not go, he does not attribute its annoying behaviour to
sin, he does not say, You are a wicked motorcar, and I shall not give you
any more petrol until you go. He attempts to find out what is wrong and
set it right.
  -- Bertrand Russell,
  Has Religion Made Useful Contributions to Civilization?

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 31, 2018

From @Smylers

Dave Mitchell writes​:

The test case reduces to

use warnings; 
package Foo \{ use overload '\.=' => sub  \{ return "foo"; \}; \} 
my $s = bless \[\]\, 'Foo'; 
my $x; 
$s \.= $x; 

which used to be silent, but now emits​:

Use of uninitialized value $x in concatenation \(\.\) or string 

I'll look into this further when I can find some time.

Why is it wrong for that to warn?

$x is undef, and is being supplied to the concatenation operator. Admittedly an overloaded operator that in this case doesn't care that it's undef, but why should Perl start presuming that undef-s are acceptable simply because an operator has been overloaded?

I couldn't see anything in overload or in perldiag mentioning that this warning doesn't apply to an overloaded operation. Many uses of overloading are intended to be largely transparent to the user, so they may be surprised to not get a warning that they requested.

Smylers

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 31, 2018

From zefram@fysh.org

Smylers wrote​:

Why is it wrong for that to warn?

The question is whether .= stringifies its rhs in this case. If it
stringifies, passing an actual string to the lhs overload, then it is
correct to gripe about undef. If it does not stringify, and so passes
the rhs value to the lhs overload as-is, then it is up to the overload
method to determine what operand values are valid, and .= has no business
opining on the matter.

In fact both 5.26.0 and 5.27.8 are agreed​: .= does not stringify the rhs
in this case. They do not call stringification overloads, and do not
flatten references. They pass the rhs value to the lhs overload as-is.

-zefram

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 20, 2018

From @iabyn

On Wed, Jan 31, 2018 at 01​:10​:33PM +0000, Zefram wrote​:

Smylers wrote​:

Why is it wrong for that to warn?

The question is whether .= stringifies its rhs in this case. If it
stringifies, passing an actual string to the lhs overload, then it is
correct to gripe about undef. If it does not stringify, and so passes
the rhs value to the lhs overload as-is, then it is up to the overload
method to determine what operand values are valid, and .= has no business
opining on the matter.

In fact both 5.26.0 and 5.27.8 are agreed​: .= does not stringify the rhs
in this case. They do not call stringification overloads, and do not
flatten references. They pass the rhs value to the lhs overload as-is.

Now fixed in blead with​:

commit af39014
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Sat Feb 10 15​:27​:59 2018 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Feb 19 22​:06​:49 2018 +0000

  redo magic/overload handing in pp_multiconcat
 
  The way pp_multiconcat handles things like tieing and overloading
  doesn't work very well at the moment. There's a lot of code to handle
  edge cases, and there are still open bugs.
 
  The basic algorithm in pp_multiconcat is to first stringify (i.e. call
  SvPV() on) *all* args, then use the obtained values to calculate the total
  length and utf8ness required, then do a single SvGROW and copy all the
  bytes from all the args.
 
  This ordering is wrong when variables with visible side effects, such as
  tie/overload, are encountered. The current approach is to stringify args
  up until such an arg is encountered, concat all args up until that one
  together via the normal fast route, then jump to a special block of code
  which concats any remaining args one by one the "hard" way, handling
  overload etc.
 
  This is problematic because we sometimes need to go back in time. For
  example in ($undef . $overloaded), we're supposed to call
  $overloaded->concat($undef, reverse=1)
  so to speak, but by the time of the method call, we've already tried to
  stringify $undef and emitted a spurious 'uninit var' warning.
 
  The new approach taken in this commit is to​:
 
  1) Bail out of the stringify loop under a greater range of problematical
  variable classes - namely we stop when encountering *anything* which
  might cause external effects, so in addition to tied and overloaded vars,
  we now stop for any sort of get magic, or any undefined value where
  warnings are in scope.
 
  2) If we bail out, we throw away any stringification results so far,
  and concatenate *all* args the slow way, even ones we're already
  stringified. This solves the "going back in time" problem mentioned above.
  It's safe because the only vars that get processed twice are ones for which
  the first stringification could have no side effects.
 
  The slow concat loop now uses S_do_concat(), which is a new static inline
  function which implements the main body of pp_concat() - so they share
  identical code.
 
  An intentional side-effect of this commit is to fix three tickets​:
 
  RT #132783
  RT #132827
  RT #132595
 
  so tests for them are included in this commit.
 
  One effect of this commit is that string concatenation of magic or
  undefined vars will now be slower than before, e.g.
 
  "pid=$$"
  "value=$undef"
 
  but they will probably still be faster than before pp_multiconcat was
  introduced.

--
print+qq&$}$"$/$s$,$a$d$g$s$@​$.$q$,$​:$.$q$^$,$@​$a$$;$.$q$m&if+map{m,^\d{0\,},,${$​::{$'}}=chr($"+=$&amp;||1)}q&10m22,42}6​:17a22.3@​3;^2dg3q/s"&=~m*\d\*.*g

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 20, 2018

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

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

No branches or pull requests

1 participant