[Patch, libfortran, 3/3] Update file position lazily

[Patch, libfortran, 3/3] Update file position lazily

[Janne Blomqvist]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

Hi,

libgfortran maintains a position flag which is used by the
INQUIRE(POSITION=...) statement. Currently we update this flag after
every IO statement. For unbuffered IO this is somewhat tedious, as
figuring out whether we're at the beginning of a file or the end
requires at least two syscalls. The attached patch moves this checking
to the inquire implementation, which is certainly less frequently
invoked than READ or WRITE.

Also, I think I've found a small standards conformance bug. From F2008
(N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
without changing its position." and "If the file has been repositioned
since the connection, the scalar-default-char-variable
is assigned a processor-dependent value, which shall not be REWIND
unless the file is positioned at its initial
point and shall not be APPEND unless the file is positioned so that its
endfile record is the next record or at its
terminal point if it has no endfile record.
"

If my understanding of the above is correct, returning ASIS is
incorrent unless the position is unchanged since the OPEN statement.
Currently we return ASIS by default if it's neither REWIND nor APPEND.
So the patch changes the implementation to return the
processor-dependent value UNSPECIFIED in this case.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

2011-10-18  Janne Blomqvist  <jb@gcc.gnu.org>

	* io/inquire.c (inquire_via_unit): Check whether we're at the
	beginning or end if the position is unspecified. If the position
	is not one of the 3 standard ones, return unspecified.
	* io/io.h (update_position): Remove prototype.
	* io/transfer.c (next_record): Set the position to unspecified,
	letting inquire figure it out more exactly when needed.
	* io/unit.c (update_position): Remove function.


testsuite ChangeLog:

2011-10-18  Janne Blomqvist  <jb@gcc.gnu.org>

	* gfortran.dg/inquire_5.f90: Update testcase to match the standard
	and current implementation.


-- 
Janne Blomqvist

[-- Attachment #2: lazypos.diff --]
[-- Type: text/x-patch, Size: 4755 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90 b/gcc/testsuite/gfortran.dg/inquire_5.f90
index fe107a1..064f96d 100644
--- a/gcc/testsuite/gfortran.dg/inquire_5.f90
+++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
@@ -1,11 +1,10 @@
 ! { dg-do run { target fd_truncate } }
-! { dg-options "-std=legacy" }
 !
 ! pr19314 inquire(..position=..) segfaults
 ! test by Thomas.Koenig@online.de
 !         bdavis9659@comcast.net
       implicit none
-      character*20 chr
+      character(len=20) chr
       open(7,STATUS='SCRATCH')
       inquire(7,position=chr)
       if (chr.NE.'ASIS') CALL ABORT
@@ -31,7 +30,8 @@
       write(7,*)'this is another record'
       backspace(7)
       inquire(7,position=chr)
-      if (chr.NE.'ASIS') CALL ABORT
+      if (chr.eq.'ASIS' .or. chr .eq. 'REWIND' &
+           .or. chr .eq. 'APPEND') CALL ABORT
       rewind(7)
       inquire(7,position=chr)
       if (chr.NE.'REWIND') CALL ABORT
diff --git a/libgfortran/io/inquire.c b/libgfortran/io/inquire.c
index 252f29f..fb525ca 100644
--- a/libgfortran/io/inquire.c
+++ b/libgfortran/io/inquire.c
@@ -418,24 +418,36 @@ inquire_via_unit (st_parameter_inquire *iqp, gfc_unit * u)
       if (u == NULL || u->flags.access == ACCESS_DIRECT)
         p = undefined;
       else
-        switch (u->flags.position)
-          {
-             case POSITION_REWIND:
-               p = "REWIND";
-               break;
-             case POSITION_APPEND:
-               p = "APPEND";
-               break;
-             case POSITION_ASIS:
-               p = "ASIS";
-               break;
-             default:
-               /* if not direct access, it must be
-                  either REWIND, APPEND, or ASIS.
-                  ASIS seems to be the best default */
-               p = "ASIS";
-               break;
-          }
+	{
+	  /* If the position is unspecified, check if we can figure
+	     out whether it's at the beginning or end.  */
+	  if (u->flags.position == POSITION_UNSPECIFIED)
+	    {
+	      gfc_offset cur = stell (u->s);
+	      if (cur == 0)
+		u->flags.position = POSITION_REWIND;
+	      else if (cur != -1 && (ssize (u->s) == cur))
+		u->flags.position = POSITION_APPEND;
+	    }
+	  switch (u->flags.position)
+	    {
+	    case POSITION_REWIND:
+	      p = "REWIND";
+	      break;
+	    case POSITION_APPEND:
+	      p = "APPEND";
+	      break;
+	    case POSITION_ASIS:
+	      p = "ASIS";
+	      break;
+	    default:
+	      /* If the position has changed and is not rewind or
+		 append, it must be set to a processor-dependent
+		 value.  */
+	      p = "UNSPECIFIED";
+	      break;
+	    }
+	}
       cf_strcpy (iqp->position, iqp->position_len, p);
     }
 
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 37353d7..23f07ca 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -608,9 +608,6 @@ internal_proto(get_unit);
 extern void unlock_unit (gfc_unit *);
 internal_proto(unlock_unit);
 
-extern void update_position (gfc_unit *);
-internal_proto(update_position);
-
 extern void finish_last_advance_record (gfc_unit *u);
 internal_proto (finish_last_advance_record);
 
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 26263ae..062f80e 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -3343,9 +3343,10 @@ next_record (st_parameter_dt *dtp, int done)
 
   if (!is_stream_io (dtp))
     {
-      /* Keep position up to date for INQUIRE */
+      /* Since we have changed the position, set it to unspecified so
+	 that INQUIRE(POSITION=) knows it needs to look into it.  */
       if (done)
-	update_position (dtp->u.p.current_unit);
+	dtp->u.p.current_unit->flags.position = POSITION_UNSPECIFIED;
 
       dtp->u.p.current_unit->current_record = 0;
       if (dtp->u.p.current_unit->flags.access == ACCESS_DIRECT)
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 1d36214..b4d10cd 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -706,26 +706,6 @@ close_units (void)
 }
 
 
-/* update_position()-- Update the flags position for later use by inquire.  */
-
-void
-update_position (gfc_unit *u)
-{
-  /* If unit is not seekable, this makes no sense (and the standard is
-     silent on this matter), and thus we don't change the position for
-     a non-seekable file.  */
-  gfc_offset cur = stell (u->s);
-  if (cur == -1)
-    return;
-  else if (cur == 0)
-    u->flags.position = POSITION_REWIND;
-  else if (ssize (u->s) == cur)
-    u->flags.position = POSITION_APPEND;
-  else
-    u->flags.position = POSITION_ASIS;
-}
-
-
 /* High level interface to truncate a file, i.e. flush format buffers,
    and generate an error or set some flags.  Just like POSIX
    ftruncate, returns 0 on success, -1 on failure.  */

Re: [Patch, libfortran, 3/3] Update file position lazily

[Mikael Morin]

On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
> Also, I think I've found a small standards conformance bug. From F2008
> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
> without changing its position." and "If the file has been repositioned
> since the connection, the scalar-default-char-variable
> is assigned a processor-dependent value, which shall not be REWIND
> unless the file is positioned at its initial
> point and shall not be APPEND unless the file is positioned so that its
> endfile record is the next record or at its
> terminal point if it has no endfile record.
> "
> 
> If my understanding of the above is correct, returning ASIS is
> incorrent unless the position is unchanged since the OPEN statement.
> Currently we return ASIS by default if it's neither REWIND nor APPEND.
> So the patch changes the implementation to return the
> processor-dependent value UNSPECIFIED in this case.
> 
If my reading is correct, returning ASIS is as valid as returning UNSPECIFIED 
("processor-dependent"). I have a preference for UNSPECIFIED and see your 
patch as OK, but shouldn't it be avoided if it breaks backwards compatibility?

I'm also afraid of testsuite changes of the following kind.
Was there no reason for the "-std=legacy"?

diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90 
b/gcc/testsuite/gfortran.dg/inquire_5.f90
index fe107a1..064f96d 100644
--- a/gcc/testsuite/gfortran.dg/inquire_5.f90
+++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
@@ -1,11 +1,10 @@
 ! { dg-do run { target fd_truncate } }
-! { dg-options "-std=legacy" }
 !

Mikael

Re: [Patch, libfortran, 3/3] Update file position lazily

[Janne Blomqvist]

On Sat, Oct 29, 2011 at 01:48, Mikael Morin <mikael.morin@sfr.fr> wrote:
> On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
>> Also, I think I've found a small standards conformance bug. From F2008
>> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
>> without changing its position." and "If the file has been repositioned
>> since the connection, the scalar-default-char-variable
>> is assigned a processor-dependent value, which shall not be REWIND
>> unless the file is positioned at its initial
>> point and shall not be APPEND unless the file is positioned so that its
>> endfile record is the next record or at its
>> terminal point if it has no endfile record.
>> "
>>
>> If my understanding of the above is correct, returning ASIS is
>> incorrent unless the position is unchanged since the OPEN statement.
>> Currently we return ASIS by default if it's neither REWIND nor APPEND.
>> So the patch changes the implementation to return the
>> processor-dependent value UNSPECIFIED in this case.
>>
> If my reading is correct, returning ASIS is as valid as returning UNSPECIFIED
> ("processor-dependent"). I have a preference for UNSPECIFIED and see your
> patch as OK, but shouldn't it be avoided if it breaks backwards compatibility?

My thinking was that the first sentence I quoted would prohibit ASIS
even though it's not explicitly forbidden in the second quoted
sentence. Fixing the implementation would thus be correcting a
standards-conformance bug.

FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
case could be made for using the same. Comments?

> I'm also afraid of testsuite changes of the following kind.
> Was there no reason for the "-std=legacy"?
>
> diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90
> b/gcc/testsuite/gfortran.dg/inquire_5.f90
> index fe107a1..064f96d 100644
> --- a/gcc/testsuite/gfortran.dg/inquire_5.f90
> +++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
> @@ -1,11 +1,10 @@
>  ! { dg-do run { target fd_truncate } }
> -! { dg-options "-std=legacy" }
>  !

I changed the declaration of "chr" from "character*20" to
"character(len=20)" which made std=legacy unnecessary. As the testcase
doesn't test any legacy functionality per se, I though this change
would slightly simplify it. See also

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40881

which mass-added std=legacy to a number of testcases (including this
one) as a result of some frontend warnings changes.

Thanks for the reviews!

-- 
Janne Blomqvist

Re: [Patch, libfortran, 3/3] Update file position lazily

[Mikael Morin]

On Saturday 29 October 2011 10:09:07 Janne Blomqvist wrote:
> On Sat, Oct 29, 2011 at 01:48, Mikael Morin <mikael.morin@sfr.fr> wrote:
> > On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
> >> Also, I think I've found a small standards conformance bug. From F2008
> >> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
> >> without changing its position." and "If the file has been repositioned
> >> since the connection, the scalar-default-char-variable
> >> is assigned a processor-dependent value, which shall not be REWIND
> >> unless the file is positioned at its initial
> >> point and shall not be APPEND unless the file is positioned so that its
> >> endfile record is the next record or at its
> >> terminal point if it has no endfile record.
> >> "
> >> 
> >> If my understanding of the above is correct, returning ASIS is
> >> incorrent unless the position is unchanged since the OPEN statement.
> >> Currently we return ASIS by default if it's neither REWIND nor APPEND.
> >> So the patch changes the implementation to return the
> >> processor-dependent value UNSPECIFIED in this case.
> > 
> > If my reading is correct, returning ASIS is as valid as returning
> > UNSPECIFIED ("processor-dependent"). I have a preference for UNSPECIFIED
> > and see your patch as OK, but shouldn't it be avoided if it breaks
> > backwards compatibility?
> 
> My thinking was that the first sentence I quoted would prohibit ASIS
> even though it's not explicitly forbidden in the second quoted
> sentence. Fixing the implementation would thus be correcting a
> standards-conformance bug.
Well, the first sentence does impose the value to be ASIS in case the position 
hasn't changed, but it does not impose the value not to be ASIS in case the 
position has changed.
On the other hand, let's think about the use cases: if we are returning ASIS 
even if the position has changed, we can't use that value reliably to tell 
that the position hasn't changed.
Thus, I think your patch is OK with the following changes.

> 
> FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
> case could be made for using the same. Comments?
Let's go for UNDEFINED then.

> 
> > I'm also afraid of testsuite changes of the following kind.
> > Was there no reason for the "-std=legacy"?
> > 
> > diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90
> > b/gcc/testsuite/gfortran.dg/inquire_5.f90
> > index fe107a1..064f96d 100644
> > --- a/gcc/testsuite/gfortran.dg/inquire_5.f90
> > +++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
> > @@ -1,11 +1,10 @@
> >  ! { dg-do run { target fd_truncate } }
> > -! { dg-options "-std=legacy" }
> >  !
> 
> I changed the declaration of "chr" from "character*20" to
> "character(len=20)" which made std=legacy unnecessary. As the testcase
> doesn't test any legacy functionality per se, I though this change
> would slightly simplify it. See also
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40881
> 
> which mass-added std=legacy to a number of testcases (including this
> one) as a result of some frontend warnings changes.
OK for that part.

>
> @@ -31,7 +30,8 @@
>        write(7,*)'this is another record'
>        backspace(7)
>        inquire(7,position=chr)
> -      if (chr.NE.'ASIS') CALL ABORT
> +      if (chr.eq.'ASIS' .or. chr .eq. 'REWIND' &
> +           .or. chr .eq. 'APPEND') CALL ABORT
I think it's better to keep the more restrictive:
         if (chr.NE.'UNDEFINED') CALL ABORT
Just in case we have a memory leak some day, which makes us return some junk 
here.

Please give the other some time to comment on our discussion before 
committing.
Thanks for the patch.

Mikael

Re: [Patch, libfortran, 3/3] Update file position lazily

[Mikael Morin]

On Saturday 29 October 2011 14:43:22 Mikael Morin wrote:
> > FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
> > case could be made for using the same. Comments?
> 
> Let's go for UNDEFINED then.
On second thought, UNSPECIFIED is better as UNDEFINED is for another case.

Re: [Patch, libfortran, 3/3] Update file position lazily

[Janne Blomqvist]

On Sat, Oct 29, 2011 at 18:35, Mikael Morin <mikael.morin@sfr.fr> wrote:
> On Saturday 29 October 2011 14:43:22 Mikael Morin wrote:
>> > FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
>> > case could be made for using the same. Comments?
>>
>> Let's go for UNDEFINED then.
> On second thought, UNSPECIFIED is better as UNDEFINED is for another case.

Hmm, indeed, on second thought I agree as well.

Further comparisons:

pathf95 3.2.99: UNKNOWN

pgf95 11.2-0: REWIND (which is clearly wrong, also inquire_5.f90
failed earlier because trying to get the position of a direct access
file returned ASIS while the standard requires UNDEFINED in this
case).


-- 
Janne Blomqvist

Re: [Patch, libfortran, 3/3] Update file position lazily

[Janne Blomqvist]

On Sun, Oct 30, 2011 at 01:29, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Sat, Oct 29, 2011 at 18:35, Mikael Morin <mikael.morin@sfr.fr> wrote:
>> On Saturday 29 October 2011 14:43:22 Mikael Morin wrote:
>>> > FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
>>> > case could be made for using the same. Comments?
>>>
>>> Let's go for UNDEFINED then.
>> On second thought, UNSPECIFIED is better as UNDEFINED is for another case.
>
> Hmm, indeed, on second thought I agree as well.

I just committed all the 3 parts of this patch series. Parts 1 and 2
verbatim, and 3 also verbatim except with the following for
inquire_5.f90:

Index: gcc/testsuite/gfortran.dg/inquire_5.f90
===================================================================
--- gcc/testsuite/gfortran.dg/inquire_5.f90     (revision 180700)
+++ gcc/testsuite/gfortran.dg/inquire_5.f90     (working copy)
@@ -1,11 +1,10 @@
 ! { dg-do run { target fd_truncate } }
-! { dg-options "-std=legacy" }
 !
 ! pr19314 inquire(..position=..) segfaults
 ! test by Thomas.Koenig@online.de
 !         bdavis9659@comcast.net
       implicit none
-      character*20 chr
+      character(len=20) chr
       open(7,STATUS='SCRATCH')
       inquire(7,position=chr)
       if (chr.NE.'ASIS') CALL ABORT
@@ -31,7 +30,7 @@
       write(7,*)'this is another record'
       backspace(7)
       inquire(7,position=chr)
-      if (chr.NE.'ASIS') CALL ABORT
+      if (chr .NE. 'UNSPECIFIED') CALL ABORT
       rewind(7)
       inquire(7,position=chr)
       if (chr.NE.'REWIND') CALL ABORT


(That is, test the returned value explicitly rather than test for
standards conformance as in the original patch)


-- 
Janne Blomqvist