Closed p5pRT closed 12 years ago
While writing tests for another bug I discovered a bug in the stacked
file tests.
When -T\, -t or -B are used in a stacked file tests then too much is
removed from the stack.
Example:
#!/usr/bin/perl -l use strict; use warnings; use Test::More tests => 2;
my $count;
my $t;
{
$count = 0;
for my $m ("c"\, "d") {
if ($count == 0) {
$t = -t -e $^X ? 0 : "bar";
}
elsif ($count == 1) {
is($m\, "d"\, "-t -e \$^X did not remove too many values
from the stack");
}
$count++;
}
$count = 0;
for my $m ("e"\, "f") {
if ($count == 0) {
$t = -T -e $^X ? 0 : "baz";
}
elsif ($count == 1) {
is($m\, "f"\, "-T -e \$^X did not remove too many values
from the stack");
}
$count++;
}
}
__END__
The output of this on blead (81b00ac76d16334c61edb1e9a4a364d00d0db5c6)
and perl-5.10.0:
1..2 not ok 1 - -t -e $^X did not remove too many values from the stack # Failed test '-t -e $^X did not remove too many values from the stack' # at /tmp/filetest.pl line 12. # got: 'bar' # expected: 'd' not ok 2 - -T -e $^X did not remove too many values from the stack # Failed test '-T -e $^X did not remove too many values from the stack' # at /tmp/filetest.pl line 23. # got: 'baz' # expected: 'f' # Looks like you failed 2 tests of 2.
A test case that is fatal:
#!/usr/bin/perl -l
use strict; use warnings; use Test::More tests => 1;
my $count;
my $t;
{
$count = 0;
for my $m ("g"\, "h") {
if ($count == 0) {
my $foo;
$foo->{bar} = -T -e $^X ? 0 : "baz";
}
elsif ($count == 1) {
is($m\, "f"\, "-T -e \$^X did not remove too many values
from the stack");
}
$count++;
}
}
__END__
Output: 1..1 Use of freed value in iteration at /tmp/filetest.pl line 19. # Looks like your test exited with 2 before it could output anything.
I had a talk with Jan Dubois about this (because he fixed a similar
bug with -u/-g/-k on windows) and this are his notes:
\
stacked file tests
\
filename on the stack\, it leaves the result of the test only.
\
STACKED_FTEST_CHECK and either return early with a FALSE value\, or
continue checking
\
test in the sequence
\
is nothing there in this case
\
is hidden in my_stat() in doio.c (my_stat_flags in 5.12)
\
filetest is executed. So how are you going to make the check now?
\
something like that.
\
stacked file tests but I don't know how complicated that is..
Best regards\,
Bram
Fairly sure that these should produce the same result:
$ ./perl -Ilib -wE 'say "Okay" if -w -T "perl.c"' Okay $ ./perl -Ilib -wE 'say "Okay" if -T -w "perl.c"' Use of uninitialized value in -T at -e line 1.
$ ./perl -Ilib -V Summary of my perl5 (revision 5 version 13 subversion 4) configuration: Commit id: f1dcae2ca2c256c755eeec79c4e7d4d5b9cf658f Platform: osname=linux\, osvers=2.6.27.24-170.2.68.fc10.i686\, archname=i686-linux uname='linux tweety.homeip.net 2.6.27.24-170.2.68.fc10.i686 #1 smp wed may 20 23:10:16 edt 2009 i686 i686 i386 gnulinux ' config_args='-des -Dusedevel' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef use64bitint=undef\, use64bitall=undef\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\, optimize='-O2'\, cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.3.2 20081105 (Red Hat 4.3.2-7)'\, gccosandvers='' intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=1234 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=12 ivtype='long'\, ivsize=4\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=4\, prototype=define Linker and Libraries: ld='cc'\, ldflags =' -fstack-protector -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.9.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.9' 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'
Characteristics of this binary (from libperl): Compile-time options: PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP PERL_USE_DEVEL USE_LARGE_FILES USE_PERLIO USE_PERL_ATOF Built under linux Compiled at Sep 5 2010 19:57:59 @INC: lib /usr/local/lib/perl5/site_perl/5.13.4/i686-linux /usr/local/lib/perl5/site_perl/5.13.4 /usr/local/lib/perl5/5.13.4/i686-linux /usr/local/lib/perl5/5.13.4 /usr/local/lib/perl5/site_perl .
On Mon Aug 23 11:51:51 2010\, p5p@perl.wizbit.be wrote:
I had a talk with Jan Dubois about this (because he fixed a similar
bug with -u/-g/-k on windows) and this are his notes: \It happens because pp_fttty and pp_fttext don't know about
stacked file tests \The problem is that the stacked filetest doesn't leave the
filename on the stack\, it leaves the result of the test only. \So the next test in the sequence will pop the result with
STACKED_FTEST_CHECK and either return early with a FALSE value\, or
continue checking \The checking will have to rely on PL_laststatval for the next
test in the sequence \But the stat buffer doesn't contain the information that -T needs \ So it always pops something from the stack\, even though there
is nothing there in this case \I see. Is it fixable? Or should I create a bug report for it? \ For pp_ftis is this hard to see because the actual stack logic
is hidden in my_stat() in doio.c (my_stat_flags in 5.12) \Please create a bug report\, I'm not sure how you can fix it easily. \ Because the filename is gone by the time the second stacked
filetest is executed. So how are you going to make the check now?
The file name is in PL_statname. The last gv is in PL_statgv. -T and -B can already handle *_. They just need to be taught to do so in stacked mode instead of popping something off the stack.
(There does seem to be another bug\, though. The code is not setting PL_laststype when called with a gv. And I think a nonexistent file name will cause the internal stat status to get out of sync\, as PL_last_stat_val and PL_statcache are not set until after the file is opened\, but PL_laststype is set before. Ouch\, there is so much wrong with this code!)
\
And extending my_stat() is not an option because -T is *expensive* \ It actually opens the file and reads the first 1024 bytes or
something like that. \I'm not sure... the fix could be to disallow -T and -t in
stacked file tests but I don't know how complicated that is..
But -t is difficult. -r -w $foo is documented as being equivalent to -r $foo && -w _. But -t doesnât work with _. We could make -t use _ when stacked\, which would be silly. I canât think of a case where stacked -t is actually useful\, which makes it hard to decide how it should work.
Actually\, forget half of that. -t could use PL_statgv\, too. We just need to document that -t -r is not exactly equivalent to -r && -t _.
Should -t -r "filename" be an error\, or equivalent to -r "filename" && -t "filename"?
--
Father Chrysostomos
The RT System itself - Status changed from 'new' to 'open'
On Wed Jan 11 22:33:40 2012\, sprout wrote:
On Mon Aug 23 11:51:51 2010\, p5p@perl.wizbit.be wrote:
I had a talk with Jan Dubois about this (because he fixed a similar
bug with -u/-g/-k on windows) and this are his notes: \It happens because pp_fttty and pp_fttext don't know about
stacked file tests \The problem is that the stacked filetest doesn't leave the
filename on the stack\, it leaves the result of the test only. \So the next test in the sequence will pop the result with
STACKED_FTEST_CHECK and either return early with a FALSE value\, or
continue checking \The checking will have to rely on PL_laststatval for the next
test in the sequence \But the stat buffer doesn't contain the information that -T needs \ So it always pops something from the stack\, even though there
is nothing there in this case \I see. Is it fixable? Or should I create a bug report for it? \ For pp_ftis is this hard to see because the actual stack logic
is hidden in my_stat() in doio.c (my_stat_flags in 5.12) \Please create a bug report\, I'm not sure how you can fix it easily. \ Because the filename is gone by the time the second stacked
filetest is executed. So how are you going to make the check now?The file name is in PL_statname. The last gv is in PL_statgv. -T and -B can already handle *_. They just need to be taught to do so in stacked mode instead of popping something off the stack.
(There does seem to be another bug\, though. The code is not setting PL_laststype when called with a gv. And I think a nonexistent file name will cause the internal stat status to get out of sync\, as PL_last_stat_val and PL_statcache are not set until after the file is opened\, but PL_laststype is set before. Ouch\, there is so much wrong with this code!)
\
And extending my_stat() is not an option because -T is *expensive* \ It actually opens the file and reads the first 1024 bytes or
something like that. \I'm not sure... the fix could be to disallow -T and -t in
stacked file tests but I don't know how complicated that is..But -t is difficult. -r -w $foo is documented as being equivalent to -r $foo && -w _. But -t doesnât work with _. We could make -t use _ when stacked\, which would be silly. I canât think of a case where stacked -t is actually useful\, which makes it hard to decide how it should work.
Actually\, forget half of that. -t could use PL_statgv\, too. We just need to document that -t -r is not exactly equivalent to -r && -t _.
Should -t -r "filename" be an error\, or equivalent to -r "filename" && -t "filename"?
Except that canât really work either\, because what would -r -t do? After all\, -t doesnât *set* the stat buffer\, either.
I propose we make stacked -t an error in 5.16. It has never worked\, itâs not clear how it should work\, and if we try to make it do something illogical weâll be stuck âsupportingâ that behaviour.
--
Father Chrysostomos
On Wed Jan 11 22:33:40 2012\, sprout wrote:
On Mon Aug 23 11:51:51 2010\, p5p@perl.wizbit.be wrote:
I had a talk with Jan Dubois about this (because he fixed a similar
bug with -u/-g/-k on windows) and this are his notes: \It happens because pp_fttty and pp_fttext don't know about
stacked file tests \The problem is that the stacked filetest doesn't leave the
filename on the stack\, it leaves the result of the test only. \So the next test in the sequence will pop the result with
STACKED_FTEST_CHECK and either return early with a FALSE value\, or
continue checking \The checking will have to rely on PL_laststatval for the next
test in the sequence \But the stat buffer doesn't contain the information that -T needs \ So it always pops something from the stack\, even though there
is nothing there in this case \I see. Is it fixable? Or should I create a bug report for it? \ For pp_ftis is this hard to see because the actual stack logic
is hidden in my_stat() in doio.c (my_stat_flags in 5.12) \Please create a bug report\, I'm not sure how you can fix it easily. \ Because the filename is gone by the time the second stacked
filetest is executed. So how are you going to make the check now?The file name is in PL_statname. The last gv is in PL_statgv. -T and -B can already handle *_. They just need to be taught to do so in stacked mode instead of popping something off the stack.
That part is fixed in commit ba8182f85.
--
Father Chrysostomos
On Wed Jan 11 22:33:40 2012\, sprout wrote:
(There does seem to be another bug\, though. The code is not setting PL_laststype when called with a gv.
That part is fixed with 5731662.
--
Father Chrysostomos
On Thu Jan 12 13:30:10 2012\, sprout wrote:
Except that canât really work either\, because what would -r -t do? After all\, -t doesnât *set* the stat buffer\, either.
I propose we make stacked -t an error in 5.16. It has never worked\, itâs not clear how it should work\, and if we try to make it do something illogical weâll be stuck âsupportingâ that behaviour.
But maybe later we could make -t work with _.
--
Father Chrysostomos
On Wed Jan 11 22:33:40 2012\, sprout wrote:
And I think a nonexistent file name will cause the internal stat status to get out of sync\, as PL_last_stat_val and PL_statcache are not set until after the file is opened\, but PL_laststype is set before.
And that part is fixed in commit ad2d99e3. See also #4253.
The only thing left in this ticket is stacked -t.
--
Father Chrysostomos
On Sat Jan 14 00:58:12 2012\, sprout wrote:
On Thu Jan 12 13:30:10 2012\, sprout wrote:
Except that canât really work either\, because what would -r -t do? After all\, -t doesnât *set* the stat buffer\, either.
I propose we make stacked -t an error in 5.16. It has never worked\, itâs not clear how it should work\, and if we try to make it do something illogical weâll be stuck âsupportingâ that behaviour.
But maybe later we could make -t work with _.
I was about to make stacked -t a compile time error\, when I realised that it would break objects with -X overloading\, which currently *do* work with stacked -t.
So I realised the real problem: When overloading is not present\, the -r in -r -w foo uses _. When overloading is present\, -w arranges for -r to receive the same argument.
So it only makes sense to me to use the latter mechanism for stacked -t.
Using the former still makes sense for other filetest operators\, because -r -w foo should only do one stat\, not two.
(Is anybody reading this?)
--
Father Chrysostomos
On Fri Jan 20 14:13:32 2012\, sprout wrote:
On Sat Jan 14 00:58:12 2012\, sprout wrote:
On Thu Jan 12 13:30:10 2012\, sprout wrote:
Except that canât really work either\, because what would -r -t do? After all\, -t doesnât *set* the stat buffer\, either.
I propose we make stacked -t an error in 5.16. It has never worked\, itâs not clear how it should work\, and if we try to make it do something illogical weâll be stuck âsupportingâ that behaviour.
But maybe later we could make -t work with _.
I was about to make stacked -t a compile time error\, when I realised that it would break objects with -X overloading\, which currently *do* work with stacked -t.
So I realised the real problem: When overloading is not present\, the -r in -r -w foo uses _. When overloading is present\, -w arranges for -r to receive the same argument.
So it only makes sense to me to use the latter mechanism for stacked -t.
Using the former still makes sense for other filetest operators\, because -r -w foo should only do one stat\, not two.
On the other hand\, -t -r -w should still work\, so -r and -w should *always* pass the filehandle or file name through\, but check the stacked flag to see whether it should actually be used.
--
Father Chrysostomos
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01-20T17:13:33]
(Is anybody reading this?)
Yes\, and it's an area I've grumbled through myself. I'll try to say something tomorrow. Tonight is crammed.
-- rjbs
On Fri Jan 20 16:25:43 2012\, perl.p5p@rjbs.manxome.org wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01- 20T17:13:33]
(Is anybody reading this?)
Yes\, and it's an area I've grumbled through myself. I'll try to say something tomorrow.
And when you do so\, can you consider the attached patch? It just makes -t -e -f -l -o N dwim\, because the N argument is now passed down to each op. Those that are capable will use the last stat buffer. Those that are not (-t -T -B) will use the argument.
--
Father Chrysostomos
On Sat Jan 21 17:04:10 2012\, sprout wrote:
On Fri Jan 20 16:25:43 2012\, perl.p5p@rjbs.manxome.org wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01- 20T17:13:33]
(Is anybody reading this?)
Yes\, and it's an area I've grumbled through myself. I'll try to say something tomorrow.
And when you do so\, can you consider the attached patch? It just makes -t -e -f -l -o N dwim\, because the N argument is now passed down to each op. Those that are capable will use the last stat buffer. Those that are not (-t -T -B) will use the argument.
Except that patch needs a bit of tweaking to make -r -t dwim\, but my point is that this is doable before 5.16.
--
Father Chrysostomos
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01-21T20:04:11]
On Fri Jan 20 16:25:43 2012\, perl.p5p@rjbs.manxome.org wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01- 20T17:13:33]
(Is anybody reading this?)
Yes\, and it's an area I've grumbled through myself. I'll try to say something tomorrow.
And when you do so\, can you consider the attached patch? It just makes -t -e -f -l -o N dwim\, because the N argument is now passed down to each op. Those that are capable will use the last stat buffer. Those that are not (-t -T -B) will use the argument.
This seems like a change for the better to me. Thanks for your patience.
-- rjbs
On Mon Jan 23 18:28:42 2012\, perl.p5p@rjbs.manxome.org wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01- 21T20:04:11]
On Fri Jan 20 16:25:43 2012\, perl.p5p@rjbs.manxome.org wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-01- 20T17:13:33]
(Is anybody reading this?)
Yes\, and it's an area I've grumbled through myself. I'll try to say something tomorrow.
And when you do so\, can you consider the attached patch? It just makes -t -e -f -l -o N dwim\, because the N argument is now passed down to each op. Those that are capable will use the last stat buffer. Those that are not (-t -T -B) will use the argument.
This seems like a change for the better to me. Thanks for your patience.
A modified version of it is now applied as 8db8f6b.
--
Father Chrysostomos
@cpansprout - Status changed from 'open' to 'resolved'
Migrated from rt.perl.org#77388 (status was 'resolved')
Searchable as RT77388$