[libgfortran] PR47778 reading two arrays of structures from namelist fails

Hi all,

The attached patch fixes this bug by resetting the pprev_nl pointer to NULL when 
the rank of the previous object read was zero.  pprev_nl is used to track 
multiple reads from the same namelist variable.

Regression tested on x86-64.

OK for trunk?  I suggest we should back port to 4.5 and possibly earlier.

Regards,

Jerry

2011-02-27  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libgfortran/47778
	* io/list_read.c (namelist_read): If pprev_nl was used during the
	previous namelist read and the rank was zero, reset the pointer to NULL
	for the next namelist read.

Jerry DeLisle wrote:
> The attached patch fixes this bug by resetting the pprev_nl pointer to 
> NULL when the rank of the previous object read was zero.  pprev_nl is 
> used to track multiple reads from the same namelist variable.
> Regression tested on x86-64.
> OK for trunk?  I suggest we should back port to 4.5 and possibly earlier.

OK for 4.6 and for 4.4/4.5.

>          }
> +      if (prev_nl && prev_nl->var_rank == 0)
> +    prev_nl = NULL;

I wonder whether a comment would help to understand the code.

> --- io/list_read.c    (revision 170543)
> +++ io/list_read.c    (working copy)
> @@ -2985,6 +2985,9 @@
>  {
>    int c;
>    char nml_err_msg[200];
> +
> +  snprintf (nml_err_msg, sizeof nml_err_msg, "Internal namelist read 
> error");

Is there a reason that you do not use:

    char nml_err_msg[200] = "Internal namelist read error";

That looks simpler too me.

Tobias

On Sun, Feb 27, 2011 at 06:39:47PM +0100, Tobias Burnus wrote:
> Jerry DeLisle wrote:
> >--- io/list_read.c    (revision 170543)
> >+++ io/list_read.c    (working copy)
> >@@ -2985,6 +2985,9 @@
> > {
> >   int c;
> >   char nml_err_msg[200];
> >+
> >+  snprintf (nml_err_msg, sizeof nml_err_msg, "Internal namelist
> >read error");
> 
> Is there a reason that you do not use:
> 
>    char nml_err_msg[200] = "Internal namelist read error";
> 
> That looks simpler too me.

The latter is much more expensive at runtime, as it also clears the
remaining 200 - 29 characters.
You could just use
  char nml_err_msg[200];
  strcpy (nml_err_msg, "Internal namelist read error");
or even
  memcpy (nml_err_msg, "Internal namelist read error",
	  sizeof "Internal namelist read error");
but snprintf above ought to be optimized into that.  Except that it isn't
(sprintf is).  So guess something that should be improved in 4.7+.

	Jakub

On 02/27/2011 09:39 AM, Tobias Burnus wrote:
> Jerry DeLisle wrote:
>> The attached patch fixes this bug by resetting the pprev_nl pointer to NULL
>> when the rank of the previous object read was zero. pprev_nl is used to track
>> multiple reads from the same namelist variable.
>> Regression tested on x86-64.
>> OK for trunk? I suggest we should back port to 4.5 and possibly earlier.
>
> OK for 4.6 and for 4.4/4.5.
>
>> }
>> + if (prev_nl && prev_nl->var_rank == 0)
>> + prev_nl = NULL;
>
> I wonder whether a comment would help to understand the code.
>

OK I can put in a comment.

>> --- io/list_read.c (revision 170543)
>> +++ io/list_read.c (working copy)
>> @@ -2985,6 +2985,9 @@
>> {
>> int c;
>> char nml_err_msg[200];
>> +
>> + snprintf (nml_err_msg, sizeof nml_err_msg, "Internal namelist read error");
>
> Is there a reason that you do not use:
>
> char nml_err_msg[200] = "Internal namelist read error";
>
> That looks simpler too me.

No particular reason, it is what we used everywhere else in the code. This 
suggests an optimization we could do everywhere. (some other time though).  I 
will give Jakub's suggestion a try.

Thanks for review.

Jerry