[perl #21656] [PATCH] read buffering in i/o

[Benjamin Goldberg]

"JüRgen BöMmels" wrote:
[snip]
> is(<FILE>, <<DATA, 'file content');

Shouldn't this be:

is(scalar <FILE>, <<DATA, 'file content');

(I don't recall whether is() provides scalar context to it's first
argument, but even if it does, it would be clearer to put the 'scalar'
in there anyway.)

[snip]
> char buf[1024];
[snip]
> len = PIO_read(interpreter, io, buf, sizeof(buf));
[snip]
> buf[len] = '\0';

This, almost certainly, should be:

buf[MIN(len, 1023)] = '\0';

After all, 'len' could concievably be 1024, if you decide to change your
earlier test to write a longer string to the temp file...

--
$a=24;split//,240513;s/\B/ => /for@@=qw(ac ab bc ba cb ca
);{push(@b,$a),($a-=6)^=1 for 2..$a/6x--$|;print "$@[$a%6
]\n";((6<=($a-=6))?$a+=$_[$a%6]-$a%6:($a=pop @b))&&redo;}

[Dan Sugalski]

At 11:01 PM -0500 3/22/03, Benjamin Goldberg wrote:
>"JüRgen BöMmels" wrote:
>[snip]
>> is(<FILE>, <<DATA, 'file content');
>
>Shouldn't this be:
>
> is(scalar <FILE>, <<DATA, 'file content');
>
>(I don't recall whether is() provides scalar context to it's first
>argument, but even if it does, it would be clearer to put the 'scalar'
>in there anyway.)
>
>[snip]
>> char buf[1024];
>[snip]
>> len = PIO_read(interpreter, io, buf, sizeof(buf));
>[snip]
>> buf[len] = '\0';
>
>This, almost certainly, should be:
>
> buf[MIN(len, 1023)] = '\0';

No, the PIO_read call should be sizeof(buf)-1. Changed as such, and
applied, as soon as the tests finish.
--
Dan

--------------------------------------"it's like this"-------------------
Dan Sugalski even samurai
d...@sidhe.org have teddy bears and even
teddy bears get drunk

[Dan Sugalski]

At 12:59 AM +0000 3/23/03, "Jürgen" "Bömmels" (via RT) wrote:
>Yet another step in PIO:
>Enabling read buffering.

A test with this starts throwing errors at t/src/list and goes on
from there--lots of double free errors. I can let the tests run to
completion if you need the list, but it looks like there's something
amiss here, and I'm not going to apply the patch for the moment.

[Juergen Boemmels]

Dan Sugalski <d...@sidhe.org> writes:

> At 12:59 AM +0000 3/23/03, "Jürgen" "Bömmels" (via RT) wrote:
> >Yet another step in PIO:
> >Enabling read buffering.
>
> A test with this starts throwing errors at t/src/list and goes on from
> there--lots of double free errors. I can let the tests run to
> completion if you need the list, but it looks like there's something
> amiss here, and I'm not going to apply the patch for the moment.

I tried to reproduce this but I failed. I tried both my working tree
and a fresh checkout with patch applied, but got no errors. It works
well with imcc and with assemble.pl (which takes an eternity on my
maschine). My system is an K6/linux/gcc-2.95.3 system.

Are you sure this problem is triggered by my patch?
Any hints?

bye
boe

[Dan Sugalski]

Hrm. I'll reapply and see if something else, since fixed, was causing
the problem.

[Dan Sugalski]

At 12:31 PM -0500 3/31/03, Dan Sugalski wrote:
>At 7:20 PM +0200 3/30/03, Juergen Boemmels wrote:
>>Dan Sugalski <d...@sidhe.org> writes:
>>
>>> At 12:59 AM +0000 3/23/03, "Jürgen" "Bömmels" (via RT) wrote:
>>> >Yet another step in PIO:
>>> >Enabling read buffering.
>>>
>>> A test with this starts throwing errors at t/src/list and goes on from
>>> there--lots of double free errors. I can let the tests run to
>>> completion if you need the list, but it looks like there's something
>>> amiss here, and I'm not going to apply the patch for the moment.
>>
>>I tried to reproduce this but I failed. I tried both my working tree
>>and a fresh checkout with patch applied, but got no errors. It works
>>well with imcc and with assemble.pl (which takes an eternity on my
>>maschine). My system is an K6/linux/gcc-2.95.3 system.
>>
>>Are you sure this problem is triggered by my patch?
>>Any hints?
>
>Hrm. I'll reapply and see if something else, since fixed, was
>causing the problem.

Alright, it's been *far* too long, but I finally took a look at this.

The problem, in a nutshell, is that lots of tests die on OS X when I
apply this patch. Linux, though, *seems* fine, until....

I go install valgrind and let it rip. Ignoring the complaints about
our GC code (it doesn't like us, and I'm not sure which complaints
are real and which are just because we're cheating like mad) the ones
that kill OS X *do* show up. There's a double-free in the teardown
code that doesn't show up when the patch isn't in.

I'm not sure why yet, bit valgrind's yipes are:

==27543== Invalid free() / delete / delete[]
==27543== at 0x4016651E: free (vg_clientfuncs.c:185)
==27543== by 0x8080774: mem_sys_free (in
/home/dsugalski/src/parrot/t/src/list_2)
==27543== by 0x8054FFF: PIO_destroy (in
/home/dsugalski/src/parrot/t/src/list_2)
==27543== by 0x8057E35: Parrot_really_destroy (in
/home/dsugalski/src/parrot/t/src/list_2)
==27543== Address 0x410148C4 is 0 bytes inside a block of size 48 free'd
==27543== at 0x4016651E: free (vg_clientfuncs.c:185)
==27543== by 0x8080774: mem_sys_free (in
/home/dsugalski/src/parrot/t/src/list_2)
==27543== by 0x80557AE: PIO_close (in
/home/dsugalski/src/parrot/t/src/list_2)
==27543== by 0x8054FF1: PIO_destroy (in
/home/dsugalski/src/parrot/t/src/list_2)

[Simon Glover]

On Tue, 29 Apr 2003, Dan Sugalski wrote:

> ==27543== Invalid free() / delete / delete[]
> ==27543== at 0x4016651E: free (vg_clientfuncs.c:185)
> ==27543== by 0x8080774: mem_sys_free (in
> /home/dsugalski/src/parrot/t/src/list_2)
> ==27543== by 0x8054FFF: PIO_destroy (in
> /home/dsugalski/src/parrot/t/src/list_2)
> ==27543== by 0x8057E35: Parrot_really_destroy (in
> /home/dsugalski/src/parrot/t/src/list_2)
> ==27543== Address 0x410148C4 is 0 bytes inside a block of size 48 free'd
> ==27543== at 0x4016651E: free (vg_clientfuncs.c:185)
> ==27543== by 0x8080774: mem_sys_free (in
> /home/dsugalski/src/parrot/t/src/list_2)
> ==27543== by 0x80557AE: PIO_close (in
> /home/dsugalski/src/parrot/t/src/list_2)
> ==27543== by 0x8054FF1: PIO_destroy (in
> /home/dsugalski/src/parrot/t/src/list_2)

Taking a quick look at io.c, I can see at least one thing that looks
dodgy. In PIO_destroy, we have a loop that does:

-------------------

for (i = 0 ; i < PIO_NR_OPEN; i++) {
if ( (io = GET_INTERP_IOD(interpreter)->table[i]) ) {
#if 0
PIO_flush(interpreter, io);
PIO_close(interpreter, io);
#endif
mem_sys_free(io);
}
}

------------------

and in PIO_close, we potentially do a mem_sys_free on io.

This is no problem in the CVS code, as the call to PIO_close is compiled
out; Juergen's patch removes the '#if 0', and thus we can potentially
wind up freeing io twice.

I don't understand the intent of the code well enough to know whether
this can be fixed simply by removing the mem_sys_free from the above,
or if the real fix is more involved.

Simon

[Juergen Boemmels]

Simon Glover <sc...@amnh.org> writes:

[valgrind]

> Taking a quick look at io.c, I can see at least one thing that looks
> dodgy. In PIO_destroy, we have a loop that does:
>
> -------------------
>
> for (i = 0 ; i < PIO_NR_OPEN; i++) {
> if ( (io = GET_INTERP_IOD(interpreter)->table[i]) ) {
> #if 0
> PIO_flush(interpreter, io);
> PIO_close(interpreter, io);
> #endif
> mem_sys_free(io);
> }
> }
>
> ------------------
>
> and in PIO_close, we potentially do a mem_sys_free on io.
>
> This is no problem in the CVS code, as the call to PIO_close is compiled
> out; Juergen's patch removes the '#if 0', and thus we can potentially
> wind up freeing io twice.

Ah there ist the problem. Without removing this #if 0 there were other
tests failing, but I dont remember exactly which one. Anyway, strange
enough I did not get any error or warning about this double free when
testing this.

> I don't understand the intent of the code well enough to know whether
> this can be fixed simply by removing the mem_sys_free from the above,
> or if the real fix is more involved.

The memory for io is allocated in the Open function by calling the
function PIO_new. So I think the correct place for freeing this memory
of io is in a PIO_... function called by the Close function. Strange
enough the function PIO_destroy does not destroy a single io, but the
whole IO-subsystem of the interpreter. This is a valid task, but does
not match what PIO_new does.

One solution would be to rename PIO_new to PIO_ParrotIO_new and have a
function PIO_ParrotIO_destroy which frees the memory and which gets
called from the Close function. The mem_sys_free(io) should be deleted
from Parrot_destroy (Maybe this function should be renamed to PIO_fini,
because it undoes the allocations of PIO_init)

bye
boe
--
Juergen Boemmels boem...@physik.uni-kl.de
Fachbereich Physik Tel: ++49-(0)631-205-2817
Universitaet Kaiserslautern Fax: ++49-(0)631-205-3906
PGP Key fingerprint = 9F 56 54 3D 45 C1 32 6F 23 F6 C7 2F 85 93 DD 47

[Melvin Smith]

At 03:06 PM 4/30/2003 +0200, Juergen Boemmels wrote:

>Simon Glover <sc...@amnh.org> writes:
>One solution would be to rename PIO_new to PIO_ParrotIO_new and have a
>function PIO_ParrotIO_destroy which frees the memory and which gets
>called from the Close function. The mem_sys_free(io) should be deleted
>from Parrot_destroy (Maybe this function should be renamed to PIO_fini,
>because it undoes the allocations of PIO_init)

Don't ask me what I was thinking when I wrote this code, you'll likely find
lots of problems in it because it was left in a state of flux when I ran
out time
to work on Parrot (hopefully temporarily).

Please don't rename anything to PIO_ParrotIO since PIO already stands
for Parrot IO. You are right, some of the methods could use renaming,
I like the PIO_finish() option.

One more thing, the intention was to make Parrot's IO objects proper
PMCs. At the time they were written PMC's were new/changing and
there was no garbage collector. I hope things are stable enough now
that someone could go ahead and implement at least a PMC wrapper class.

-Melvin

[Juergen Boemmels]

Melvin Smith <mrjol...@mindspring.com> writes:

> Please don't rename anything to PIO_ParrotIO since PIO already stands
> for Parrot IO. You are right, some of the methods could use renaming,
> I like the PIO_finish() option.

Ok, I will take that route.

> One more thing, the intention was to make Parrot's IO objects proper
> PMCs. At the time they were written PMC's were new/changing and
> there was no garbage collector. I hope things are stable enough now
> that someone could go ahead and implement at least a PMC wrapper class.

I wanted to do something like that when I'm finished with the
buffering IO, but ATM I can only work on parrot during the weekends
and sometimes not even that.

bye
boe

[Matt Fowles]

All~

I am running Win2K with MSVC and nmake. I get the following error on a
fresh check out. I did do a make realclean if it matters.

cl -nologo -Gf -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE
-DNO_STRICT -
DUSE_PERLIO -I./include -DHAS_JIT -DI386 -Fochartypes/unicode.obj -c
chartype
s/unicode.c
unicode.c
cl -nologo -Gf -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE
-DNO_STRICT -
DUSE_PERLIO -I./include -DHAS_JIT -DI386 -Fochartypes/usascii.obj -c
chartype
s/usascii.c
usascii.c
link -out:parrot.exe -nologo -nodefaultlib -release
-machine:x86 exce
ptions.obj global_setup.obj interpreter.obj parrot.obj register.obj
core_ops.ob
j core_ops_prederef.obj memory.obj packfile.obj stacks.obj string.obj
sub.obj e
ncoding.obj chartype.obj runops_cores.obj trace.obj pmc.obj key.obj
hash.obj c
ore_pmcs.obj platform.obj jit.obj jit_cpu.obj jit_debug.obj
resources.obj rx.ob
j rxstacks.obj intlist.obj list.obj embed.obj warnings.obj
packout.obj byteo
rder.obj debug.obj smallobject.obj headers.obj dod.obj method_util.obj
exit.obj
misc.obj spf_render.obj spf_vtable.obj datatypes.obj fingerprint.obj
nci.obj
cpu_dep.obj tsq.obj io/io.obj io/io_buf.obj io/io_unix.obj
io/io_win32.obj io/
io_stdio.obj classes/array.obj classes/boolean.obj classes/compiler.obj
classes/
continuation.obj classes/coroutine.obj classes/csub.obj
classes/default.obj clas
ses/eval.obj classes/intlist.obj classes/key.obj
classes/managedstruct.obj class
es/multiarray.obj classes/nci.obj classes/perlarray.obj
classes/perlhash.obj cla
sses/perlint.obj classes/perlnum.obj classes/perlstring.obj
classes/perlundef.ob
j classes/pointer.obj classes/scalar.obj classes/scratchpad.obj
classes/sub.obj
classes/unmanagedstruct.obj encodings/singlebyte.obj encodings/utf8.obj
encodi
ngs/utf16.obj encodings/utf32.obj chartypes/unicode.obj
chartypes/usascii.obj te
st_main.obj oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib
comdlg32
.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib
uuid.lib wsock
32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib msvcrt.lib
io.obj : error LNK2001: unresolved external symbol _pio_win32_layer
io_buf.obj : error LNK2001: unresolved external symbol _PIO_win32_getblksize
io_buf.obj : error LNK2001: unresolved external symbol _PIO_win32_isatty
parrot.exe : fatal error LNK1120: 3 unresolved externals
NMAKE : fatal error U1077: 'link' : return code '0x460'
Stop.

Thanks,
Matt

[Juergen Boemmels]

Why got the text of my last message stripped.
Hier is the text for io4.diff:

Ok, here is the next version of the patch.

The changes to the buffering system did not change since the last
version.

To clean up the double free error, I renamed the function PIO_destroy
to PIO_finisch because it cleans up what PIO_init creates. The only
caller of this function in interpreter.c is fixed.

The new PIO_destroy cleans up the the data allocated from PIO_new and
is only called from the Close-API call. I hope this cleans up the
double free error, but my system did not show up the original
error. I'm still trying to install valgrind.

bye
boe

[Dan Sugalski]

OS X is much, much happier. (And you have good timing, the tests were
running when you sent this) Applied, with much thanks.