PR libfortran/29568 (subrecord patch)

[Brooks Moses]

Thomas Koenig wrote:
> after quite some time, here it is:  The gfortran subrecord patch
> (aka implementing the Intel format for unformatted files).  This was
> regression-tested on i686-pc-linux-gnu.  A slightly earlier version
> (which was missing some casts) was extensively tested on 64-bit
> Linux and FreeBSD by Jerry DeLisle (thanks a lot, Jerry!)

A few minor things:

> Index: gcc/fortran/options.c
> ===================================================================
> --- gcc/fortran/options.c	(revision 119261)
> +++ gcc/fortran/options.c	(working copy)
> @@ -636,6 +636,12 @@ gfc_handle_option (size_t scode, const c
>      case OPT_frecord_marker_8:
>        gfc_option.record_marker = 8;
>        break;
> +
> +    case OPT_fmax_subrecord_length_:
> +      if (value > MAX_SUBRECORD_LENGTH || value < 1)
> +	gfc_fatal_error ("Maximum subrecord length is %d", MAX_SUBRECORD_LENGTH);
> +
> +      gfc_option.max_subrecord_length = value;

I think this error message is confusing, because you've got two 
competing definitions for "maximum subrecord length" -- the option is 
(according to the lang.opt documentation) defining what the maximum 
subrecord length is, but this is giving an error that (1) doesn't state 
clearly what's wrong, and (2) gives a value for the "maximum subrecord 
length" that isn't what the user specified in the option that defines it.

I'd suggest making "maximum subrecord length" unambiguously refer to the 
user-supplied value in the UI, never to MAX_SUBRECORD_LENGTH, and 
writing this error as "Maximum subrecord length cannot be greater than %d".

That, of course, is inappropriate for the "value < 1" case, but I think 
it makes sense either to put in a separate error message for that, or to 
note that the only possible value that's less than 1 is 0, since 
UInteger options never have negative values, and that it might make just 
as much sense to use -fmax-subrecord-length=0 as a flag that means "as 
unlimited as possible" in the same way that it means that for line 
lengths and such, and have that set the value to MAX_SUBRECORD_LENGTH.

Also, I'm somewhat surprised that there isn't a default value for 
gfc_option.max_subrecord_length added to gfc_init_options.  As best I 
can tell, if -fmax-subrecord-length is not given, then it's never 
initialized at all.

> ===================================================================
> --- gcc/fortran/gfortran.h	(revision 119261)
> +++ gcc/fortran/gfortran.h	(working copy)
> @@ -1661,11 +1661,13 @@ typedef struct
>    int fshort_enums;
>    int convert;
>    int record_marker;
> +  int max_subrecord_length;
>  }
>  gfc_option_t;
>  
>  extern gfc_option_t gfc_option;
>  
> +#define MAX_SUBRECORD_LENGTH ((int) ((1u<<31u) - 9u))

I don't think this is the clearest place to put this; there are several 
other hard limits of this sort at lines 58-60 in gfortran.h, and I would 
think this should go with them.

> ===================================================================
> --- libgfortran/libgfortran.h	(revision 119261)
> +++ libgfortran/libgfortran.h	(working copy)
> @@ -379,6 +380,7 @@ internal_proto(compile_options);
>  extern void init_compile_options (void);
>  internal_proto(init_compile_options);
>  
> +#define GFC_MAX_SUBRECORD_LENGTH (GFC_INTEGER_4_HUGE - 8)

As a curiousity question: why is this different from the value given in 
gfortran.h?

> Index: libgfortran/io/transfer.c
> ===================================================================
> --- libgfortran/io/transfer.c	(revision 119261)
> +++ libgfortran/io/transfer.c	(working copy)
> @@ -353,62 +362,169 @@ read_block_direct (st_parameter_dt *dtp,
>  	  return;
>  	}
>  
> -      nread = *nbytes;
> -      if (sread (dtp->u.p.current_unit->s, buf, &nread) != 0)
> +      to_read_record = *nbytes;
> +      have_read_record = to_read_record;
> +      if (sread (dtp->u.p.current_unit->s, buf, &have_read_record) != 0)
>  	{
>  	  generate_error (&dtp->common, ERROR_OS, NULL);
>  	  return;
>  	}
>  
> -      dtp->u.p.current_unit->strm_pos += (gfc_offset) nread; 
> -
> -      if (nread != *nbytes)  /* Short read, e.g. if we hit EOF.  */
> -	generate_error (&dtp->common, ERROR_END, NULL);	  
> +      dtp->u.p.current_unit->strm_pos += (gfc_offset) have_read_record; 
>  
> +      if (to_read_record != have_read_record)
> +	{
> +	  /* Short read,  e.g. if we hit EOF.  */
> +	  generate_error (&dtp->common, ERROR_END, NULL);
> +	  return;
> +	}
>        return;
>      }

This isn't directly related to your patch, but I wonder if this is 
related to PR 30009, in which a short record generates an erroneous 
end-of-file error.  The comments certainly would seem to imply so; "e.g. 
if we hit EOF" implies that an EOF is one example of why one might have 
a short read, so if this situation is indeed one that can _only_ arise 
in cases of end-of-file, the comment should perhaps be clarified a bit.

Speaking of which, given that you're doing so much mucking about with 
records, it's probably worth checking to see if you've accidentally 
fixed PR 30009 in the process.  :)

If you have, it may be worth adding a testsuite version of the sample 
file from that PR, to make sure it stays fixed.

- Brooks


P.S. This wasn't posted to gcc-patches@; you might want to do that when 
you commit it.