Re: [patch, fortran] New fix: PR31201 Too large unit number generates wrong code

[Jerry DeLisle <jvdelisle at verizon dot net>]

FX Coudert wrote:
> > Here is the updated patch with comments incorporated.
>
> Thanks again for your work. I review the patch, you'll find my comments 
> below. Unfortunately, I think I found a testcase on which it fails:
>
> $ cat a2.f90
>   integer :: i
>   print *, int(2_8*int(huge(0_4),kind=8)+9_8,kind=4)
>   open(2_8*int(huge(0_4),kind=8)+9_8,file="foo",iostat=i)
>   print *, i
>   end
> $ gfortran -static a2.f90 -fdump-tree-original -g -fno-range-check && 
> ./a.out
>            7
>            0
>
> The large unit number I used is such that, when converted into an int4, 
> it is a valid unit number. From the tree dump:
>
> >     _gfortran_generate_error (&open_parm.1, 5005, "Unit number in I/O 
> > statement too large");
> >     open_parm.1.common.unit = 7;
> >     _gfortran_st_open (&open_parm.1);
>
> I think the problem is that _gfortran_st_open just resets the IOSTAT 
> variable. I first thought that _gfortran_st_open should just exit early 
> if it is passed a ioparm with nonzero iostat value, but it wouldn't work 
> in the case where the IOSTAT variable was already set before by the 
> user, like in:
>
> >     i = -12
> >     open(10,file="foo",iostat=i)

Good Catch:  iostat is initialized in library start.  I will come up with a fix 
for this.
> My second suggestion is that maybe we should simply not call 
> _gfortran_st_open after _gfortran_generate_error. But I don't know how 
> to do that in the current scheme; maybe using a return value for 
> _gfortran_generate_error, storing it into a variable (that was earlier 
> initialized to 0) and conditionalize the call to _gfortran_st_open on 
> that value?
>
> A third idea, which seems better still: since iostat is always 
> overwritten (is that right?), we could initialize it to zero when we 
> start creating the ioparm structure, and the fall back to my first idea 
> (have the various library functions bail out if iostat is nonzero; maybe 
> even do that in library_start, where iostat is initialized in all 
> cases). The case of ERR, IOMSG and others remains to be seen.

I prefer this approach.  Initialize iostat when creating ioparm.  Take the 
initialization out of library start so the value generated by the front-end is 
not overwritten.  We want to just let library_start not do anything with it 
since the value needs to be carried through to after all IO operations.

If, for example in your test case, the user chooses to ignore the iostat value 
and attempts to open the unit even though it has a wrong number, thats their 
problem, we are already going out of our way to catch a stupidity here anyway 
and we are telling the program there is an error.  We could set the unit number 
to -1 as a precaution.

Regardless I will give this some thought for the next round on this patch.

> Fourth and last idea: add a flag to the structure, like 
> error_has_already_been_raised.

Maybe, I will think about it for a bit.

> I'm sorry that I have no solution for the problem. Hopefully, being 
> closer to the code, you'll have an idea of which scheme is best, or 
> think of something else. You can reach me on IRC or by email to discuss 
> about this if you wish.
>
> Thanks again, and sorry I didn't spot that on the first review (I 
> initially thought the library did bail out early if IOSTAT was 
> initialized, before I realized it wasn't possible).
>
> FX
>
>
> > A test case will be derived from the PR. (I have tested the heck out 
> > of it)
>
> Good.
>
> > +   ERROR_SHORT_RECORD,
> > +   ERROR_CORRUPT_FILE,
> > +   ERROR_LAST            /* Not a real error, the last error # + 1.  */
> > + }
> > + error_codes;
>
> Just to avoid any possiblity of future clash, I'd prefer these names to 
> start with IOERROR_ (and ioerror_codes).

OK

> > + /* Build code to test an error condition and call generate_error if 
> > needed.
> > +    Note: This builds calls to generate_error in the runtime library 
> > function.
> > +    The function generate_error is dependent on certain parameters in 
> > the
> > +    st_parameter_common flags to be set. (See 
> > libgfortran/runtime/error.c)
> > +    Therefore, the code to set these flags must be generated before
> > +    this function is used.  */
>
> Very good comment.
>
> > +   arg2 = build_int_cst (integer_type_node, error_code),
>
> This does not agree with:
>
> > +  gfor_fndecl_generate_error =
> > +    gfc_build_library_function_decl (get_identifier 
> > (PREFIX("generate_error")),
> > +                                    void_type_node, 3, 
> > gfc_int4_type_node,
> > +                                     pvoid_type_node, pchar_type_node);

oops!  I will fix that.
>

I hope the next patch does it.

Jerry