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

[Jerry DeLisle]

On 02/19/2011 11:38 AM, Janne Blomqvist wrote:
> On Sat, Feb 19, 2011 at 20:27, Jerry DeLisle<jvdelisle@frontier.com>  wrote:
>> On 02/19/2011 09:15 AM, Janne Blomqvist wrote:
>>>     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?
>
> I'm very suspicious about this; I see no logical reason how it could
> make sense. Perhaps a bug elsewhere, yes.

Lets see what happens with the fbuf_getc approach and we can discuss this case 
further if needed.

>
>>> 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.
>
> Good point.
>
>>
>>> 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.
>
> I suspect the fbuf_getc() approach might be faster, as in that case we
> don't need to scan through the string twice (once in fbuf_read(), once
> in read_sf()), but in any case I'm not that concerned about the
> performance. I'd just like something that is straightforward to
> understand and which makes sense, so we reduce the possibility of
> further regressions.
>

Ah yes, I see your point, we scan it over again in read_sf.  I will work up a 
revised patch and see how it does.

Thanks for comments.

Jerry