Patchwork [Fortran] PR 50109 - fix skipping multiple comment lines in namelists

login
register
mail settings
Submitter Tobias Burnus
Date Aug. 18, 2011, 9:59 a.m.
Message ID <4E4CE288.2010405@net-b.de>
Download mbox | patch
Permalink /patch/110507/
State New
Headers show

Comments

Tobias Burnus - Aug. 18, 2011, 9:59 a.m.
The patch should be rather simple and self explaining.

Build and regtested on x86-64-linux.
OK for the trunk?

I wonder how far we should backport; the program is said to work in 
4.1.2 and 4.2.5, but to fail in 4.3.6 up to the trunk. Oldest maintained 
GCC is 4.4.6. The patch is relatively simple, but as the long time 
between breakage and report shows, it does not seem to affect many users.

I think we should backport the patch at least to 4.6 and 4.5 , but one 
could also consider 4.4 or even 4.3. What do you think?

Tobias

PS: RHES 6 uses 4.4, SLES uses 4.3 with 4.5 as technical preview.
Tobias Burnus - Aug. 18, 2011, 10:21 a.m.
On 08/18/2011 11:59 AM, Tobias Burnus wrote:
> The patch should be rather simple and self explaining.

I just realized that I forgot to add more context to the diff. Thus, 
below the patch itself with some more lines (copied from 
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50109#c4).

Without, it looks less obvious than it actually is.

One simply jumps again to the top of the "do { } while()" after removing 
the "!" line. The LIBERROR_END check is superfluous as it is already 
tested when moving to the top of "do {".

I was thinking of using "continue" or goto next, but the former will 
still check the while() condition and I think the latter looks more 
complicated/cluttered than the "c='\n';".

Tobias

@@ -345,28 +345,19 @@ eat_separator (st_parameter_dt *dtp)
           do
             {
               if ((c = next_char (dtp)) == EOF)
                   return LIBERROR_END;
               if (c == '!')
                 {
                   err = eat_line (dtp);
                   if (err)
                     return err;
-                 if ((c = next_char (dtp)) == EOF)
-                   return LIBERROR_END;
-                 if (c == '!')
-                   {
-                     err = eat_line (dtp);
-                     if (err)
-                       return err;
-                     if ((c = next_char (dtp)) == EOF)
-                       return LIBERROR_END;
-                   }
+                 c = '\n';
                 }
             }
           while (c == '\n' || c == '\r' || c == ' ' || c == '\t');
           unget_char (dtp, c);
Mikael Morin - Aug. 18, 2011, 1:37 p.m.
On Thursday 18 August 2011 11:59:36 Tobias Burnus wrote:
> The patch should be rather simple and self explaining.
It is indeed with the extra context.

> 
> Build and regtested on x86-64-linux.
> OK for the trunk?
Yes.

> 
> I wonder how far we should backport; the program is said to work in
> 4.1.2 and 4.2.5, but to fail in 4.3.6 up to the trunk. Oldest maintained
> GCC is 4.4.6. The patch is relatively simple, but as the long time
> between breakage and report shows, it does not seem to affect many users.
> 
> I think we should backport the patch at least to 4.6 and 4.5 ,
makes sense.
> but one could also consider 4.4 or even 4.3. What do you think?
It looks like a real bug, with a simple fix, so I would say 4.4 too.
About 4.3, as it is unmaintained, we are not supposed to backport patches for 
it?

Mikael
Steve Kargl - Aug. 18, 2011, 3:27 p.m.
On Thu, Aug 18, 2011 at 03:37:44PM +0200, Mikael Morin wrote:
> On Thursday 18 August 2011 11:59:36 Tobias Burnus wrote:
> > 
> > I wonder how far we should backport; the program is said to work in
> > 4.1.2 and 4.2.5, but to fail in 4.3.6 up to the trunk. Oldest maintained
> > GCC is 4.4.6. The patch is relatively simple, but as the long time
> > between breakage and report shows, it does not seem to affect many users.
> > 
> > I think we should backport the patch at least to 4.6 and 4.5 ,
> > makes sense.
> > but one could also consider 4.4 or even 4.3. What do you think?
> It looks like a real bug, with a simple fix, so I would say 4.4 too.
> About 4.3, as it is unmaintained, we are not supposed to backport patches for 
> it?

I agree with Mikael that the patch looks fairly simple (so
a bad side effect should not occur).  A backport to 4.4 is
fine with me.

Patch

2011-08-18  Tobias Burnus  <burnus@net-b.de>

	PR fortran/50109
	* io/list_read.c (eat_separator): Fix skipping over "!" lines.

2011-08-18  Tobias Burnus  <burnus@net-b.de>

	PR fortran/50109
	* gfortran.dg/namelist_73.f90: New.

diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index 01272d0..11a35c9 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -351,16 +351,7 @@  eat_separator (st_parameter_dt *dtp)
 		  err = eat_line (dtp);
 		  if (err)
 		    return err;
-		  if ((c = next_char (dtp)) == EOF)
-		    return LIBERROR_END;
-		  if (c == '!')
-		    {
-		      err = eat_line (dtp);
-		      if (err)
-			return err;
-		      if ((c = next_char (dtp)) == EOF)
-			return LIBERROR_END;
-		    }
+		  c = '\n';
 		}
 	    }
 	  while (c == '\n' || c == '\r' || c == ' ' || c == '\t');
--- /dev/null	2011-08-17 07:24:12.871882230 +0200
+++ gcc/gcc/testsuite/gfortran.dg/namelist_73.f90	2011-08-18 11:40:28.000000000 +0200
@@ -0,0 +1,28 @@ 
+! { dg-do run }
+!
+! PR fortran/50109
+!
+! Contributed by Jim Hanson
+!
+      program namelist_test
+
+      integer nfp
+      namelist /indata/ nfp
+
+      nfp = 99
+      open(unit=4, status='scratch')
+      write(4,'(a)') '$indata'
+      write(4,'(a)') 'NFP = 5,'
+      write(4,'(a)') "!  "
+      write(4,'(a)') "! "
+      write(4,'(a)') "!  "
+      write(4,'(a)') '/'
+
+      rewind(4)
+      read (4,nml=indata)
+      close(4)
+
+!      write(*,*) nfp
+      if (nfp /= 5) call abort()
+
+      end