search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.99k stars 559 forks source link

Bleadperl v5.17.0-424-gd24ca0c breaks ABIGAIL/Regexp-Common-2011121001.tar.gz #12184

Closed p5pRT closed 12 years ago

p5pRT commented 12 years ago

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

Searchable as RT113682$

p5pRT commented 12 years ago

From @andk

git bisect

commit d24ca0c5f11250dcd2552c84a048bda5786ba8d1 Author​: David Mitchell \davem@​iabyn\.com Date​: Sun Mar 18 15​:53​:40 2012 +0000

  Fix up runtime regex codeblocks.

sample fail reports

| ABIGAIL/Regexp-Common-2011121001.tar.gz | http​://www.cpantesters.org/cpan/report/9a242998-b6a4-11e1-856a-3c003af89482 | | DLAND/Regexp-Assemble-0.35.tar.gz | http​://www.cpantesters.org/cpan/report/6572e2ce-b6b4-11e1-933a-e6fb39f89482 | | ASAVIGE/Acme-EyeDrops-1.60.tar.gz | http​://www.cpantesters.org/cpan/report/c0ee7c16-b76e-11e1-a9ad-27273af89482 |

perl -V

Summary of my perl5 (revision 5 version 17 subversion 0) configuration​:
  Commit id​: d24ca0c5f11250dcd2552c84a048bda5786ba8d1   Platform​:   osname=linux\, osvers=3.2.0-2-amd64\, archname=x86_64-linux-thread-multi   uname='linux k83 3.2.0-2-amd64 #1 smp mon may 21 17​:45​:41 utc 2012 x86_64 gnulinux '   config_args='-Dprefix=/home/src/perl/repoperls/installed-perls/perl/v5.17.0-424-gd24ca0c/9980 -Dmyhostname=k83 -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Duseithreads -Uuselongdouble -DDEBUGGING=-g'   hint=recommended\, useposix=true\, d_sigaction=define   useithreads=define\, usemultiplicity=define   useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef   use64bitint=define\, use64bitall=define\, uselongdouble=undef   usemymalloc=n\, bincompat5005=undef   Compiler​:   cc='cc'\, ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\,   optimize='-O2 -g'\,   cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'   ccversion=''\, gccversion='4.7.0'\, 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/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib   libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lpthread -lc   perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc   libc=\, so=so\, useshrplib=false\, libperl=libperl.a   gnulibc_version='2.13'   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'

Characteristics of this binary (from libperl)​:   Compile-time options​: HAS_TIMES MULTIPLICITY PERLIO_LAYERS   PERL_DONT_CREATE_GVSV PERL_IMPLICIT_CONTEXT   PERL_MALLOC_WRAP PERL_PRESERVE_IVUV PERL_USE_DEVEL   USE_64_BIT_ALL USE_64_BIT_INT USE_ITHREADS   USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE   USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO   USE_PERL_ATOF USE_REENTRANT_API   Built under linux   Compiled at Jun 16 2012 00​:53​:55   @​INC​:   /home/src/perl/repoperls/installed-perls/perl/v5.17.0-424-gd24ca0c/9980/lib/site_perl/5.17.0/x86_64-linux-thread-multi   /home/src/perl/repoperls/installed-perls/perl/v5.17.0-424-gd24ca0c/9980/lib/site_perl/5.17.0   /home/src/perl/repoperls/installed-perls/perl/v5.17.0-424-gd24ca0c/9980/lib/5.17.0/x86_64-linux-thread-multi   /home/src/perl/repoperls/installed-perls/perl/v5.17.0-424-gd24ca0c/9980/lib/5.17.0   .

-- andreas

p5pRT commented 12 years ago

From @iabyn

On Sat\, Jun 16\, 2012 at 10​:22​:55AM -0700\, Andreas J. Koenig via RT wrote​:

git bisect ---------- commit d24ca0c5f11250dcd2552c84a048bda5786ba8d1 Author​: David Mitchell \davem@​iabyn\.com Date​: Sun Mar 18 15​:53​:40 2012 +0000

Fix up runtime regex codeblocks\.

sample fail reports ------------------- | ABIGAIL/Regexp-Common-2011121001.tar.gz | http​://www.cpantesters.org/cpan/report/9a242998-b6a4-11e1-856a-3c003af89482 |

Having had an initial look at the first failure in Regexp​::Common\, it seems to be an issue with "use re 'eval'" in conjunction with an unholy complex of tieing and overloading.

It seems to be roughly equivalent to the following code snippet​:

  use overload '""' => sub { qr/(?{})/ };   my $x = bless [];   "" =~ /^$x/;

which succeeds with 5.16.0 but dies in bleed\, with​:

  Eval-group not allowed at runtime\, use re 'eval'...

If the string overloading is changed to qr overloading\, i.e.

  use overload 'qr"' => sub { qr/(?{})/ };

then the outcome is reversed​: bleed succeeds\, but 5.16.0 dies!

My feeling is that the bleed behaviour is right\, although I'm open to discussion.

My basic rule for 'use re eval' in my re_eval jumbo fix\, is​: if code blocks appear in a literal regex\, its ok; if qr//'s containing code blocks appear in a list of things to be concatenated at runtime\, then its ok​:   $r = qr/(?{})/; /abc$r/

if the code block appears by any other route\, then it's a runtime invasion\, and 'use re eval' is required.

As regards overloading\, I would think that if the stringify overload method is used\, then the regex engine should expect a string\, and any returned qr// would be stringified\, and not examined for code blocks etc. Conversely\, I would expect qr overloading to return a qr\, and thus for any of its codeblocks to be used as-is.

But its all a bit of grey (nay\, muddy) area.

-- Please note that ash-trays are provided for the use of smokers\, whereas the floor is provided for the use of all patrons.   -- Bill Royston

p5pRT commented 12 years ago

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

p5pRT commented 12 years ago

From @iabyn

On Sun\, Jun 17\, 2012 at 11​:08​:37PM +0100\, Dave Mitchell wrote​:

On Sat\, Jun 16\, 2012 at 10​:22​:55AM -0700\, Andreas J. Koenig via RT wrote​:

git bisect ---------- commit d24ca0c5f11250dcd2552c84a048bda5786ba8d1 Author​: David Mitchell \davem@​iabyn\.com Date​: Sun Mar 18 15​:53​:40 2012 +0000

Fix up runtime regex codeblocks\.

sample fail reports ------------------- | ABIGAIL/Regexp-Common-2011121001.tar.gz | http​://www.cpantesters.org/cpan/report/9a242998-b6a4-11e1-856a-3c003af89482 |

Having had an initial look at the first failure in Regexp​::Common\, it seems to be an issue with "use re 'eval'" in conjunction with an unholy complex of tieing and overloading.

Having looked further\, I've now mostly fixed this with the commit shown below. With that commit\, Regexp-Assemble-0.35 passes all its tests. Acme-EyeDrops-1.60 still fails like mad\, but I have no intention of investigating that further; I value my sanity.

That leaves Regexp-Common-2011121001. Applying the following diff to it​:

Inline Patch ```diff --- Regexp-Common-2011121001.orig/lib/Regexp/Common/balanced.pm 2010-02-23 16:59:52.000000000 +0000 +++ Regexp-Common-2011121001/lib/Regexp/Common/balanced.pm 2012-06-19 10:58:59.076427686 +0100 @@ -58,6 +58,7 @@ } $cache {$start} {$finish} = $count; + use re 'eval'; $Regexp::Common::balanced [$count] = qr/@re/; } ```

along with the fix to blead, makes all tests pass apart from t/test_i.t. This is failing because the "" overload function in Regexp​::Common​::Entry (around line 261 of Regexp/Common.pm) takes a returned qr// object that has embedded code (such as qr/(?{}/)\, and does a substitution on it\, converting it from a regex object into a plain string. Later that string is interpolated back into another pattern\, and fails with the 'Eval-group not allowed at runtime' error. The fact that this ever worked was a bug a in earlier versions of perl.

I haven't tried to fix this myself\, as I don't understand the code well enough\, but I would speculate that you may need to convert the stringifed pattern back into a qr object\, e.g.

  $pat = s/.../.../; # whoops stringified   {   use re 'eval';   $pat = qr/$pat/;   }

Although whether that is safe or sensible\, I don't know.

commit e03b874abb33f381913aa76467fee51dbdc78263 Author​: David Mitchell \davem@​iabyn\.com AuthorDate​: Mon Jun 18 22​:40​:25 2012 +0100 Commit​: David Mitchell \davem@​iabyn\.com CommitDate​: Tue Jun 19 12​:23​:26 2012 +0100

  overloading​: make qr fallback to "" better  
  With the re_eval jumbo fix\, the behaviour of overloaded objects in   runtime patterns\, such /^$overloaded/ has changed\, such that the stringify   overload ("") no longer avoids the need for 'use re "eval"'​: for example\,  
  use overload "" => sub { qr/(??{1})/ }   my $o = bless [];   "1" =~ /^$o/;  
  works in 5.16.0\, but dies with "Eval-group not allowed" in blead.  
  Change this back to the former behaviour\, such that if qr and concat   ops aren't overloaded\, then use "" overloading\, and if the return from   that is a qr object\, extract any code blocks from it.  
  This is achieved by​:   * moving the concat/stringify code ahead of the regex block extraction   code\,   * making the overloaded stringify call be explicit (rather than   being invoked implicitly by sv_catsv())\,   * looping to re-apply overloading to any object returned by "".   * applying those last two steps in the case of a single arg too  
  This is a partial fix for   [perl #113682] Bleadperl v5.17.0-424-gd24ca0c breaks   ABIGAIL/Regexp-Common-2011121001.tar.gz

M regcomp.c M t/re/pat.t

-- Never do today what you can put off till tomorrow.

p5pRT commented 12 years ago

From @abigail

On Tue\, Jun 19\, 2012 at 12​:50​:26PM +0100\, Dave Mitchell wrote​:

That leaves Regexp-Common-2011121001. Applying the following diff to it​:

--- Regexp-Common-2011121001.orig/lib/Regexp/Common/balanced.pm 2010-02-23 16​:59​:52.000000000 +0000 +++ Regexp-Common-2011121001/lib/Regexp/Common/balanced.pm 2012-06-19 10​:58​:59.076427686 +0100 @​@​ -58\,6 +58\,7 @​@​ }

 $cache \{$start\} \{$finish\} = $count;

+ use re 'eval'; $Regexp​::Common​::balanced [$count] = qr/@​re/; }

This looks like the right thing to do\, as @​re can have embedded '(??{ })' constructs.

Or course\, the right thing to do is check the Perl version\, and if the version is >= 5.10\, use a recursive regexp (although that has its own can of worms\, but that's unrelated to the issue here).

along with the fix to blead\, makes all tests pass apart from t/test_i.t. This is failing because the "" overload function in Regexp​::Common​::Entry (around line 261 of Regexp/Common.pm) takes a returned qr// object that has embedded code (such as qr/(?{}/)\, and does a substitution on it\, converting it from a regex object into a plain string. Later that string is interpolated back into another pattern\, and fails with the 'Eval-group not allowed at runtime' error. The fact that this ever worked was a bug a in earlier versions of perl.

I haven't tried to fix this myself\, as I don't understand the code well enough\, but I would speculate that you may need to convert the stringifed pattern back into a qr object\, e.g.

$pat = s/\.\.\./\.\.\./; \# whoops stringified
\{
use re 'eval';
$pat = qr/$pat/;
\}

Although whether that is safe or sensible\, I don't know.

I expect that​:

  $pat =~ s/.../.../ unless ref $pat;

ought to do the trick. But it's been a while since I last looked at this code. I will look at this\, but it has to wait after OSCON\, as I'll be away for vacation starting next week (and this week is a busy work week).

Regards\,

Abigail

p5pRT commented 12 years ago

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