[libfortran] PR45629 Get rid of setjmp/longjmp in the IO library

On 11/01/2010 04:30 PM, Janne Blomqvist wrote:
> Hi,
>
> the attached patch gets rid of the setjmp()/longjmp() based EOF
> handling in list directed reads. It also fixes PR 45629.
>
> Tested on x86_64-unknown-linux-gnu, Ok for trunk?  I understand stage
> 1 just ended, and as EOF handling has been a rich source of tricky
> bugs in the IO library, I'm perfectly Ok with postponing this until
> 4.7.

Overall I like the patch.

I am concerned about the changes to st_parameter_dt.  To be on the safe side I 
wonder if you should do something like this?  (Maybe I worry too much.  ;) )


Otherwise patch is OK.

Jerry

On Tue, Nov 2, 2010 at 05:27, Jerry DeLisle <jvdelisle@frontier.com> wrote:
> On 11/01/2010 04:30 PM, Janne Blomqvist wrote:
>>
>> Hi,
>>
>> the attached patch gets rid of the setjmp()/longjmp() based EOF
>> handling in list directed reads. It also fixes PR 45629.
>>
>> Tested on x86_64-unknown-linux-gnu, Ok for trunk?  I understand stage
>> 1 just ended, and as EOF handling has been a rich source of tricky
>> bugs in the IO library, I'm perfectly Ok with postponing this until
>> 4.7.
>
> Overall I like the patch.
>
> I am concerned about the changes to st_parameter_dt.  To be on the safe side
> I wonder if you should do something like this?  (Maybe I worry too much.  ;)
> )

Yes, I think you worry too much in this case. The part of
st_parameter_dt I have modified is never touches by the frontend, the
frontend just allocates space on the stack for the library to use. A
condensed version is

struct st_parameter_dt
{

>
> Index: io.h
> ===================================================================
> --- io.h        (revision 164907)
> +++ io.h        (working copy)
> @@ -435,7 +427,7 @@ typedef struct st_parameter_dt
>          unsigned format_not_saved : 1;
>          /* 14 unused bits.  */
>
> -         char last_char;
> +         char old_last_char; /* Remove when ABI breaks.  */
>          char nml_delim;
>
>          int repeat_count;
> @@ -446,7 +438,7 @@ typedef struct st_parameter_dt
>          char *scratch;
>          char *line_buffer;
>          struct format_data *fmt;
> -         jmp_buf *eof_jump;
> +         jmp_buf *eof_jump; /* Remove when ABI breaks.  */
>          namelist_info *ionml;
>          /* A flag used to identify when a non-standard expanded namelist
> read
>             has occurred.  */
> @@ -456,6 +448,7 @@ typedef struct st_parameter_dt
>             largest kind.  */
>          char value[32];
>          GFC_IO_INT size_used;
> +         int last_char;
>        } p;
>       /* This pad size must be equal to the pad_size declared in
>         trans-io.c (gfc_build_io_library_fndecls).  The above structure
>
> Otherwise patch is OK.
>
> Jerry
>

Huh, accidentally hit the send button... continuing where I left off.

On Tue, Nov 2, 2010 at 10:27, Janne Blomqvist <blomqvist.janne@gmail.com> wrote:
> On Tue, Nov 2, 2010 at 05:27, Jerry DeLisle <jvdelisle@frontier.com> wrote:
>> On 11/01/2010 04:30 PM, Janne Blomqvist wrote:
>>>
>>> Hi,
>>>
>>> the attached patch gets rid of the setjmp()/longjmp() based EOF
>>> handling in list directed reads. It also fixes PR 45629.
>>>
>>> Tested on x86_64-unknown-linux-gnu, Ok for trunk?  I understand stage
>>> 1 just ended, and as EOF handling has been a rich source of tricky
>>> bugs in the IO library, I'm perfectly Ok with postponing this until
>>> 4.7.
>>
>> Overall I like the patch.
>>
>> I am concerned about the changes to st_parameter_dt.  To be on the safe side
>> I wonder if you should do something like this?  (Maybe I worry too much.  ;)
>> )
>
> Yes, I think you worry too much in this case. The part of
> st_parameter_dt I have modified is never touched by the frontend, the
> frontend just allocates space on the stack for the library to use. A
> condensed version is

struct st_parameter_dt
{
    // stuff
  union
    {
      struct
        {
           // private part of struct, including last_char and jmp_buf
        } p;
      /* This pad size must be equal to the pad_size declared in
         trans-io.c (gfc_build_io_library_fndecls).  The above structure
         must be smaller or equal to this array.  */
      char pad[16 * sizeof (char *) + 32 * sizeof (int)];
    } u;
    // some more stuff
}

That is, as long as the "p" struct is smaller than the array "pad", no
worries. My patch does actually reduce the size of the "p" struct;
changing last_char to int increases the size by 3 bytes, but removing
the jmp_buf pointer reduces it by 4 or 8 bytes + possible alignment
changes.

>> Otherwise patch is OK.

Thanks for the review. Do you agree with Steve that it's Ok to commit
it now and we have time to fix any issues before release?

On 11/02/2010 01:35 AM, Janne Blomqvist wrote:
> Huh, accidentally hit the send button... continuing where I left off.
>
--- snip ---
>>> Otherwise patch is OK.
>
> Thanks for the review. Do you agree with Steve that it's Ok to commit
> it now and we have time to fix any issues before release?
>
>

Yes, plenty of time to address any issues. OK to commit.

Jerry

On Tue, Nov 2, 2010 at 13:49, Jerry DeLisle <jvdelisle@frontier.com> wrote:
> On 11/02/2010 01:35 AM, Janne Blomqvist wrote:
>>
>> Huh, accidentally hit the send button... continuing where I left off.
>>
> --- snip ---
>>>>
>>>> Otherwise patch is OK.
>>
>> Thanks for the review. Do you agree with Steve that it's Ok to commit
>> it now and we have time to fix any issues before release?
>>
>>
>
> Yes, plenty of time to address any issues. OK to commit.

Committed as r166180.