[patch, gfortran] PR48298 DTIO, implement size=

[patch, gfortran] PR48298 DTIO, implement size=

[Jerry DeLisle]

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

Hi all,

The attached patch enables the size= specifier in a READ statement to work with 
child DTIO procedures. This is accomplished by moving the size_used variable 
from the dtp structure to the gfc_unit structure so that the accumulation of 
bytes during READ is carried across the procedures via the UNIT.

As far as I know, this is the last DTIO patch needed for full implementation and 
will close the PR.

After this patch is committed I plan to prepare a clean up patch to reorganize 
the dtp structure and clear at least one TODO related to stream IO. The 
follow-on patch will bump the major version number of libgfortran to 4.

Regression tested on x86-64-linux. New test case attached.

OK for trunk?

Jerry

2016-10-17  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/48298
	* io/io.h: Move size_used from dtp to unit structure. Add bool
	has_size to unit structure.
	* read.c (read_x): Use has_size and size_used.
	* transfer.c (read_sf_internal): Likewise. (read_sf): Likewise.
	(read_block_form): Likewise. (read_block_form4): Likewise.
	(data_transfer_init): If parent, initialize the size variables.
	(finalize_transfer): Set the size variable using size_used in
	gfc_unit. (write_block): Delete bogus/dead code.



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

diff --git a/libgfortran/ChangeLog b/libgfortran/ChangeLog
index bfda86df..f20c5106 100644
--- a/libgfortran/ChangeLog
+++ b/libgfortran/ChangeLog
@@ -1,3 +1,15 @@
+2016-10-17  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
+
+	PR fortran/48298
+	* io/io.h: Move size_used from dtp to unit structure. Add bool
+	has_size to unit structure.
+	* read.c (read_x): Use has_size and size_used.
+	* transfer.c (read_sf_internal): Likewise. (read_sf): Likewise.
+	(read_block_form): Likewise. (read_block_form4): Likewise.
+	(data_transfer_init): If parent, initialize the size variables.
+	(finalize_transfer): Set the size variable using size_used in
+	gfc_unit. (write_block): Delete bogus/dead code.
+
 2016-10-16  Janne Blomqvist  <jb@gcc.gnu.org>
 
 	PR libfortran/48587
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index aaacc089..edc520a9 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -514,7 +514,6 @@ typedef struct st_parameter_dt
 	     large enough to hold a complex value (two reals) of the
 	     largest kind.  */
 	  char value[32];
-	  GFC_IO_INT size_used;
 	  formatted_dtio fdtio_ptr;
 	  unformatted_dtio ufdtio_ptr;
 	} p;
@@ -650,6 +649,8 @@ typedef struct gfc_unit
   /* DTIO Parent/Child procedure, 0 = parent, >0 = child level.  */
   int child_dtio;
   int last_char;
+  bool has_size;
+  GFC_IO_INT size_used;
 }
 gfc_unit;
 
diff --git a/libgfortran/io/read.c b/libgfortran/io/read.c
index f8d5b72e..d72cdb37 100644
--- a/libgfortran/io/read.c
+++ b/libgfortran/io/read.c
@@ -1282,8 +1282,9 @@ read_x (st_parameter_dt *dtp, int n)
     } 
 
  done:
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) n;
+  if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
+      dtp->u.p.current_unit->has_size)
+    dtp->u.p.current_unit->size_used += (GFC_IO_INT) n;
   dtp->u.p.current_unit->bytes_left -= n;
   dtp->u.p.current_unit->strm_pos += (gfc_offset) n;
 }
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 2232417a..e5805772 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -267,8 +267,9 @@ read_sf_internal (st_parameter_dt *dtp, int * length)
 
   dtp->u.p.current_unit->bytes_left -= *length;
 
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) *length;
+  if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
+      dtp->u.p.current_unit->has_size)
+    dtp->u.p.current_unit->size_used += (GFC_IO_INT) *length;
 
   return base;
 
@@ -397,8 +398,9 @@ read_sf (st_parameter_dt *dtp, int * length)
 
   dtp->u.p.current_unit->bytes_left -= n;
 
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) n;
+  if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
+      dtp->u.p.current_unit->has_size)
+    dtp->u.p.current_unit->size_used += (GFC_IO_INT) n;
 
   /* We can't call fbuf_getptr before the loop doing fbuf_getc, because
      fbuf_getc might reallocate the buffer.  So return current pointer
@@ -478,8 +480,9 @@ read_block_form (st_parameter_dt *dtp, int * nbytes)
   source = fbuf_read (dtp->u.p.current_unit, nbytes);
   fbuf_seek (dtp->u.p.current_unit, *nbytes, SEEK_CUR);
 
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) *nbytes;
+  if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
+      dtp->u.p.current_unit->has_size)
+    dtp->u.p.current_unit->size_used += (GFC_IO_INT) *nbytes;
 
   if (norig != *nbytes)
     {
@@ -536,8 +539,9 @@ read_block_form4 (st_parameter_dt *dtp, int * nbytes)
 
   dtp->u.p.current_unit->bytes_left -= *nbytes;
 
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) *nbytes;
+  if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
+      dtp->u.p.current_unit->has_size)
+    dtp->u.p.current_unit->size_used += (GFC_IO_INT) *nbytes;
 
   return source;
 }
@@ -770,9 +774,6 @@ write_block (st_parameter_dt *dtp, int length)
 	}
     }
 
-  if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used += (GFC_IO_INT) length;
-
   dtp->u.p.current_unit->strm_pos += (gfc_offset) length;
 
   return dest;
@@ -2596,9 +2597,6 @@ data_transfer_init (st_parameter_dt *dtp, int read_flag)
   if ((dtp->common.flags & IOPARM_LIBRETURN_MASK) != IOPARM_LIBRETURN_OK)
     return;
 
-  if ((cf & IOPARM_DT_HAS_SIZE) != 0)
-    dtp->u.p.size_used = 0;  /* Initialize the count.  */
-
   dtp->u.p.current_unit = get_unit (dtp, 1);
 
   if (dtp->u.p.current_unit == NULL)
@@ -2674,6 +2672,18 @@ data_transfer_init (st_parameter_dt *dtp, int read_flag)
 	return;
     }
 
+  if (dtp->u.p.current_unit->child_dtio == 0)
+    {
+      if ((cf & IOPARM_DT_HAS_SIZE) != 0)
+	{
+	  dtp->u.p.current_unit->has_size = true;
+	  /* Initialize the count.  */
+	  dtp->u.p.current_unit->size_used = 0;
+	}
+      else
+	dtp->u.p.current_unit->has_size = false;
+    }
+
   /* Check the action.  */
 
   if (read_flag && dtp->u.p.current_unit->flags.action == ACTION_WRITE)
@@ -3772,7 +3782,7 @@ finalize_transfer (st_parameter_dt *dtp)
     return;
 
   if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)
-    *dtp->size = dtp->u.p.size_used;
+    *dtp->size = dtp->u.p.current_unit->size_used;
 
   if (dtp->u.p.eor_condition)
     {



[-- Attachment #3: dtio_17.f90 --]
[-- Type: text/x-fortran, Size: 2184 bytes --]

! { dg-do run }
! PR48298, this tests function of size= specifier with DTIO.
MODULE p
  USE ISO_FORTRAN_ENV
  TYPE :: person
    CHARACTER (LEN=20) :: name
    INTEGER(4) :: age
    CONTAINS
      procedure :: pwf
      procedure :: prf
      GENERIC :: WRITE(FORMATTED) => pwf
      GENERIC :: READ(FORMATTED) => prf
  END TYPE person
CONTAINS
  SUBROUTINE pwf (dtv,unit,iotype,vlist,iostat,iomsg)
    CLASS(person), INTENT(IN) :: dtv
    INTEGER, INTENT(IN) :: unit
    CHARACTER (LEN=*), INTENT(IN) :: iotype
    INTEGER, INTENT(IN) :: vlist(:)
    INTEGER, INTENT(OUT) :: iostat
    CHARACTER (LEN=*), INTENT(INOUT) :: iomsg
    CHARACTER (LEN=30) :: udfmt
    INTEGER :: myios

    iomsg = "SUCCESS"
    iostat=0
    if (iotype.eq."DT") then
      WRITE(unit, FMT = '(a20,i2)', IOSTAT=iostat, advance='no') dtv%name, dtv%age
      if (iostat.ne.0) iomsg = "Fail PWF DT"
    endif
    if (iotype.eq."LISTDIRECTED") then
      WRITE(unit, '(*(g0))', IOSTAT=iostat) dtv%name, dtv%age
      if (iostat.ne.0) iomsg = "Fail PWF DT"
    endif
  END SUBROUTINE pwf

  SUBROUTINE prf (dtv,unit,iotype,vlist,iostat,iomsg)
    CLASS(person), INTENT(INOUT) :: dtv
    INTEGER, INTENT(IN) :: unit
    CHARACTER (LEN=*), INTENT(IN) :: iotype
    INTEGER, INTENT(IN) :: vlist(:)
    INTEGER, INTENT(OUT) :: iostat
    CHARACTER (LEN=*), INTENT(INOUT) :: iomsg
    CHARACTER (LEN=30) :: udfmt
    INTEGER :: myios
    real :: areal
    udfmt='(*(g0))'
    iomsg = "SUCCESS"
    iostat=0
    if (iotype.eq."DT") then
      READ(unit, FMT = '(a20,i2)', IOSTAT=iostat) dtv%name, dtv%age
      if (iostat.ne.0) iomsg = "Fail PWF DT"
    endif
  END SUBROUTINE prf

END MODULE p

PROGRAM test
  USE p
  implicit none
  TYPE (person) :: chairman
  integer(4) :: rl, tl, kl, thesize

  chairman%name="Charlie"
  chairman%age=62

  open(28, status='scratch')
  write(28, '(i10,i10,DT,i15,DT,i12)') rl, kl, chairman, rl, chairman, tl
  rewind(28)
  chairman%name="bogus"
  chairman%age=99
  !print *, chairman
  read(28, '(i10,i10,DT,i15,DT,i12)', advance='no', size=thesize) rl, &
                          & kl, chairman, rl, chairman, tl
  if (thesize.ne.91) call abort
  close(28)
END PROGRAM test

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Steve Kargl]

On Mon, Oct 17, 2016 at 06:02:52PM -0700, Jerry DeLisle wrote:
> Hi all,
> 
> The attached patch enables the size= specifier in a READ statement to work with 
> child DTIO procedures. This is accomplished by moving the size_used variable 
> from the dtp structure to the gfc_unit structure so that the accumulation of 
> bytes during READ is carried across the procedures via the UNIT.
> 
> As far as I know, this is the last DTIO patch needed for full implementation and 
> will close the PR.
> 
> After this patch is committed I plan to prepare a clean up patch to reorganize 
> the dtp structure and clear at least one TODO related to stream IO. The 
> follow-on patch will bump the major version number of libgfortran to 4.
> 
> Regression tested on x86-64-linux. New test case attached.
> 
> OK for trunk?

Lookd good to me.

> 	* transfer.c (read_sf_internal): Likewise. (read_sf): Likewise.
> 	(read_block_form): Likewise. (read_block_form4): Likewise.


You can simplify this by

 	* transfer.c (read_sf_internal, read_sf, read_block_form,
	read_block_form4): Likewise.

-- 
Steve

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Christophe Lyon]

Hi,


On 18 October 2016 at 04:45, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Oct 17, 2016 at 06:02:52PM -0700, Jerry DeLisle wrote:
>> Hi all,
>>
>> The attached patch enables the size= specifier in a READ statement to work with
>> child DTIO procedures. This is accomplished by moving the size_used variable
>> from the dtp structure to the gfc_unit structure so that the accumulation of
>> bytes during READ is carried across the procedures via the UNIT.
>>
>> As far as I know, this is the last DTIO patch needed for full implementation and
>> will close the PR.
>>
>> After this patch is committed I plan to prepare a clean up patch to reorganize
>> the dtp structure and clear at least one TODO related to stream IO. The
>> follow-on patch will bump the major version number of libgfortran to 4.
>>
>> Regression tested on x86-64-linux. New test case attached.
>>
>> OK for trunk?
>
> Lookd good to me.
>
>>       * transfer.c (read_sf_internal): Likewise. (read_sf): Likewise.
>>       (read_block_form): Likewise. (read_block_form4): Likewise.
>
>

Since this was committed (r241294), I'm seeing regressions on
arm and aarch64 targets.

The list is a bit long, so it's probably simpler you look at:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241306/report-build-info.html
Then click on the red "REGRESSED" in the gfortran column.

Thanks,

Christophe


> You can simplify this by
>
>         * transfer.c (read_sf_internal, read_sf, read_block_form,
>         read_block_form4): Likewise.
>
> --
> Steve

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Andreas Schwab]

At line 74 of file /opt/gcc/gcc-20161019/gcc/testsuite/gfortran.dg/dtio_17.f90 (unit = 28, file = '/tmp/gfortrantmpQF10b7')
Fortran runtime error: Bad value during integer read

Error termination. Backtrace:
FAIL: gfortran.dg/dtio_17.f90   -O1  execution test

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Jerry DeLisle]

On 10/19/2016 01:59 AM, Christophe Lyon wrote:
> Hi,
>
>
> On 18 October 2016 at 04:45, Steve Kargl
> <sgk@troutmask.apl.washington.edu> wrote:
>> On Mon, Oct 17, 2016 at 06:02:52PM -0700, Jerry DeLisle wrote:
>>> Hi all,
>>>
>>> The attached patch enables the size= specifier in a READ statement to work with
>>> child DTIO procedures. This is accomplished by moving the size_used variable
>>> from the dtp structure to the gfc_unit structure so that the accumulation of
>>> bytes during READ is carried across the procedures via the UNIT.

--- snip ---
>
> Since this was committed (r241294), I'm seeing regressions on
> arm and aarch64 targets.
>
> The list is a bit long, so it's probably simpler you look at:
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241306/report-build-info.html
> Then click on the red "REGRESSED" in the gfortran column.
>

--- snip ---

The patch changes nothing related to any particular target. If I were to hazard 
a guess I would think the tests are linking against a wrong version of the 
library, the follow on patch (not yet committed) is bumping the libgfortran 
major version number.

I do not have access to a target machine unless there is a way to remote in.  Is 
there one in the gcc compile farm?

Jerry

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Jerry DeLisle]

On 10/19/2016 04:04 AM, Andreas Schwab wrote:
> At line 74 of file /opt/gcc/gcc-20161019/gcc/testsuite/gfortran.dg/dtio_17.f90 (unit = 28, file = '/tmp/gfortrantmpQF10b7')
> Fortran runtime error: Bad value during integer read
>
> Error termination. Backtrace:
> FAIL: gfortran.dg/dtio_17.f90   -O1  execution test
>
> Andreas.
>

See my reply to Christophe post on arm aarch64.

Could you try compiling manually the failing test with gfortran -static and then 
run it to see what happens. Also what target is this failing on.

Jerry

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Andreas Schwab]

On Okt 19 2016, Jerry DeLisle <jvdelisle@charter.net> wrote:

> The patch changes nothing related to any particular target. If I were to
> hazard a guess I would think the tests are linking against a wrong version
> of the library, the follow on patch (not yet committed) is bumping the
> libgfortran major version number.

There is only one version of the library, the one just built.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Andreas Schwab]

On Okt 19 2016, Jerry DeLisle <jvdelisle@charter.net> wrote:

> Could you try compiling manually the failing test with gfortran -static
> and then run it to see what happens.

That does not change anything.

> Also what target is this failing on.

aarch64 and m68k.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch, gfortran] PR48298 DTIO, implement size=

[Jerry DeLisle]

On 10/19/2016 08:20 AM, Andreas Schwab wrote:
> On Okt 19 2016, Jerry DeLisle <jvdelisle@charter.net> wrote:
>
>> Could you try compiling manually the failing test with gfortran -static
>> and then run it to see what happens.
>
> That does not change anything.
>
>> Also what target is this failing on.
>
> aarch64 and m68k.
>
> Andreas.
>

Dominique has discovered my flaw, the integers are being used not initialized 
and can happen to be large negative values that need 11 bytes in the format to 
hold the values.

I will commit a change to the test case shortly.

Jerry