[patch, libgfortran] PR47694 [4.3/4.4/4.5/4.6 Regression] Fortran read from named pipe fails

[Jerry DeLisle]

On 02/19/2011 09:15 AM, Janne Blomqvist wrote:
> On Sat, Feb 19, 2011 at 04:18, Jerry DeLisle<jvdelisle@frontier.com>  wrote:
>> Hi folks,
>>
>> The problem here is that when a read from a named pipe reads more than one
>> line worth, the formatted IO buffer holds onto the data.  When fbuf_read is
>> subsequently invoked, a sread is dutifully called and it waits for the next
>> read completion.  In the meantime, the next line of data is already in the
>> buffer so the read waits for another "enter".  The reads get out of sync.
>>   The problem does not occur with regular files because there is either
>> always data available from the file to read or an EOF occurs and in both
>> those cases sread returns.
>
> Thanks for the explanation; So I think the fundamental issue is that
> in order to handle pipes (and perhaps other stuff like terminals too?)
> correctly, we need to consume all the bytes in the buffer before
> asking for more via sread.
>
>> The patch fixes this by scanning the buffer under certain conditions looking
>> for an End-of-Line (or EOR). If EOR is found, there is no need to sread
>> again from the pipe.  This patch does add a little overhead from the tight
>> scan loop, but will save some system calls.
>>
>> Regression tested on x86-64.  I would appreciate others testing please. I do
>> not know how to create a specific named pipe test in the test suite. See the
>> PR for the original test.
>>
>> OK for trunk?
>
>     if (oldpos + *len>  oldact)
>       {
> +      /* See if we have a pending EOR in here.  */
> +      if (*len>  oldact)
>
> This second conditional makes no sense. What matters is if the current
> position in the buffer + len goes past the active bytes, which is
> exactly the previous conditional. Thus, does the patch work if you
> remove that conditional and check for EOR unconditionally if oldpos +
> *len>  oldact?

I thought it made no sense either. But without it, fmt_t_2.f90 fails.  It is the 
only regression I found when testing this patch.  I determined that condition by 
experiment.  I looked at the values of oldpos, oldact, *len for various test 
cases to see what was unique about that one in particular. I then tried 
different combinations of conditions until it worked for the piped case and this 
case and no regressions. I suppose we could study that case more explicitly, but 
it is unique. Maybe a bug elsewhere?

>
> Secondly, you check for "\n" or "\r", but I think you need to check
> that a "\r" is followed by a "\n" before you can say that you're at
> EOR.
>

It does not matter because either way the result is we read what is in the 
buffer and sf_read looks for the \r followed by \n. All we are doing here is 
avoiding an sread and I wanted to address the case where there is a \r without a 
\n. Of course I just realized I can artificially test this on my machine here 
and I will do so.

> I wonder, would it perhaps be cleaner to create a new function, say
> something like
>
> static inline uchar *
> fbuf_getptr (gfc_unit * u)
> {
>    return (uchar*) u->fbuf->buf + u->fbuf->pos;
> }
>
> use this in read_sf() to get a pointer to eventually return, then loop
> forwards one character at a time using fbuf_getc() checking for
> EOR/EOF and other conditions/errors along the way.
>
> What do you think?
>

This alternate approach could work, but I think the numerous calls to fbuf_getc 
even if inlined will be less efficient then the approach I have which is a 
fairly tight loop.

Do you want to test and compare? It will take me a little while to modify and 
test it.  Also, how exactly should we test for performance? cat bigfile 
 >tmpfifo? or just read a big file? and time it.

Jerry