diff mbox

[libgfortran] PR59700 and PR59764 Misleading/buggy runtime error message

Message ID 52D17BCC.7060209@charter.net
State New
Headers show

Commit Message

Jerry DeLisle Jan. 11, 2014, 5:13 p.m. UTC 
The attached patch fixes both these bugs, combining Steve's patch and mine.
Recent fixes of memory leaks placed the free_line before the generation of the
error messages rather than after,  The item_count which identifies the read list
item involved with the error was getting cleared, resulting in a faulty error
message.  This is fixed by moving the free_line location to after the message
string is created (before issuing the error still).

The second issue is the variable item_count was being used for two separate
purposes and collided in the case of read_logical.  This was a known issue from
several years ago. Fixed by adjusting the dtp structure just a bit. I used a
spare bit for the expanded_read flag and then used the integer slot made
available for a new variable line_buffer_pos. The new variable name is an
accurate description of what it does, not confused with "item_count".

Regression tested on x86-64.

OK for trunk?  I will add the test case from PR59700 to the test suite.

Regards,

Jerry


2014-01-10  Jerry DeLisle  <jvdelisle@gcc.gnu>
	    Steven G. Kargl  <kargl@gcc.gnu.org>

	PR libfortran/59700
	PR libfortran/59764
	* io/io.h (struct st_parameter_dt): Assign expanded_read flag to
	unused bit. Define new variable line_buffer_pos.
	* io/list_read.c (free_saved, next_char, l_push_char,
	read_logical, read_real): Replace use of item_count with
	line_buffer_pos for line_buffer look ahead.
	(read_logical, read_integer, parse_real, read_real, check_type):
	Adjust location of free_line to after generating error messages
	to retain the correct item count for the message.

Comments

Jerry DeLisle Jan. 11, 2014, 5:27 p.m. UTC | #1 
My apologies please.  I forgot to mention Dominiq for his superb assistance with
this patch and testing. Many thanks!

On 01/11/2014 09:13 AM, Jerry DeLisle wrote:
> The attached patch fixes both these bugs, combining Steve's patch and mine.
> Recent fixes of memory leaks placed the free_line before the generation of the
> error messages rather than after,  The item_count which identifies the read list
> item involved with the error was getting cleared, resulting in a faulty error
> message.  This is fixed by moving the free_line location to after the message
> string is created (before issuing the error still).
> 
> The second issue is the variable item_count was being used for two separate
> purposes and collided in the case of read_logical.  This was a known issue from
> several years ago. Fixed by adjusting the dtp structure just a bit. I used a
> spare bit for the expanded_read flag and then used the integer slot made
> available for a new variable line_buffer_pos. The new variable name is an
> accurate description of what it does, not confused with "item_count".
> 
> Regression tested on x86-64.
> 
> OK for trunk?  I will add the test case from PR59700 to the test suite.
> 
> Regards,
> 
> Jerry
> 
> 
> 2014-01-10  Jerry DeLisle  <jvdelisle@gcc.gnu>
> 	    Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	PR libfortran/59700
> 	PR libfortran/59764
> 	* io/io.h (struct st_parameter_dt): Assign expanded_read flag to
> 	unused bit. Define new variable line_buffer_pos.
> 	* io/list_read.c (free_saved, next_char, l_push_char,
> 	read_logical, read_real): Replace use of item_count with
> 	line_buffer_pos for line_buffer look ahead.
> 	(read_logical, read_integer, parse_real, read_real, check_type):
> 	Adjust location of free_line to after generating error messages
> 	to retain the correct item count for the message.
>
Steve Kargl Jan. 11, 2014, 6:10 p.m. UTC | #2 
On Sat, Jan 11, 2014 at 09:27:44AM -0800, Jerry DeLisle wrote:
> My apologies please.  I forgot to mention Dominiq for his
> superb assistance with this patch and testing. Many thanks!
> 

Add Dominiq to the ChangeLog entry.  I think the patch
is OK.
diff mbox

Patch

Index: io.h
===================================================================
--- io.h	(revision 206351)
+++ io.h	(working copy)
@@ -430,7 +430,10 @@  typedef struct st_parameter_dt
 	  unsigned g0_no_blanks : 1;
 	  /* Used to signal use of free_format_data.  */
 	  unsigned format_not_saved : 1;
-	  /* 14 unused bits.  */
+	  /* A flag used to identify when a non-standard expanded namelist read
+	     has occurred.  */
+	  unsigned expanded_read : 1;
+	  /* 13 unused bits.  */
 
 	  /* Used for ungetc() style functionality. Possible values
 	     are an unsigned char, EOF, or EOF - 1 used to mark the
@@ -447,9 +450,8 @@  typedef struct st_parameter_dt
 	  char *line_buffer;
 	  struct format_data *fmt;
 	  namelist_info *ionml;
-	  /* A flag used to identify when a non-standard expanded namelist read
-	     has occurred.  */
-	  int expanded_read;
+	  /* Current position within the look-ahead line buffer.  */
+	  int line_buffer_pos;
 	  /* Storage area for values except for strings.  Must be
 	     large enough to hold a complex value (two reals) of the
 	     largest kind.  */
Index: list_read.c
===================================================================
--- list_read.c	(revision 206351)
+++ list_read.c	(working copy)
@@ -118,7 +118,7 @@  free_saved (st_parameter_dt *dtp)
 static void
 free_line (st_parameter_dt *dtp)
 {
-  dtp->u.p.item_count = 0;
+  dtp->u.p.line_buffer_pos = 0;
   dtp->u.p.line_buffer_enabled = 0;
 
   if (dtp->u.p.line_buffer == NULL)
@@ -150,15 +150,15 @@  next_char (st_parameter_dt *dtp)
     {
       dtp->u.p.at_eol = 0;
 
-      c = dtp->u.p.line_buffer[dtp->u.p.item_count];
-      if (c != '\0' && dtp->u.p.item_count < 64)
+      c = dtp->u.p.line_buffer[dtp->u.p.line_buffer_pos];
+      if (c != '\0' && dtp->u.p.line_buffer_pos < 64)
 	{
-	  dtp->u.p.line_buffer[dtp->u.p.item_count] = '\0';
-	  dtp->u.p.item_count++;
+	  dtp->u.p.line_buffer[dtp->u.p.line_buffer_pos] = '\0';
+	  dtp->u.p.line_buffer_pos++;
 	  goto done;
 	}
 
-      dtp->u.p.item_count = 0;
+      dtp->u.p.line_buffer_pos = 0;
       dtp->u.p.line_buffer_enabled = 0;
     }    
 
@@ -639,7 +639,7 @@  l_push_char (st_parameter_dt *dtp, char c)
   if (dtp->u.p.line_buffer == NULL)
     dtp->u.p.line_buffer = xcalloc (SCRATCH_SIZE, 1);
 
-  dtp->u.p.line_buffer[dtp->u.p.item_count++] = c;
+  dtp->u.p.line_buffer[dtp->u.p.line_buffer_pos++] = c;
 }
 
 
@@ -749,7 +749,7 @@  read_logical (st_parameter_dt *dtp, int length)
 	{
 	  dtp->u.p.nml_read_error = 1;
 	  dtp->u.p.line_buffer_enabled = 1;
-	  dtp->u.p.item_count = 0;
+	  dtp->u.p.line_buffer_pos = 0;
 	  return;
 	}
       
@@ -757,14 +757,17 @@  read_logical (st_parameter_dt *dtp, int length)
 
  bad_logical:
 
-  free_line (dtp);
-
   if (nml_bad_return (dtp, c))
-    return;
+    {
+      free_line (dtp);
+      return;
+    }
 
+
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
@@ -772,6 +775,7 @@  read_logical (st_parameter_dt *dtp, int length)
     eat_line (dtp);
   snprintf (message, MSGLEN, "Bad logical value while reading item %d",
 	      dtp->u.p.item_count);
+  free_line (dtp);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
   return;
 
@@ -912,9 +916,9 @@  read_integer (st_parameter_dt *dtp, int length)
   else if (c != '\n')
     eat_line (dtp);
 
-  free_line (dtp);
   snprintf (message, MSGLEN, "Bad integer for item %d in list input",
 	      dtp->u.p.item_count);
+  free_line (dtp);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
 
   return;
@@ -1297,9 +1301,9 @@  parse_real (st_parameter_dt *dtp, void *buffer, in
   else if (c != '\n')
     eat_line (dtp);
 
-  free_line (dtp);
   snprintf (message, MSGLEN, "Bad floating point number for item %d",
 	      dtp->u.p.item_count);
+  free_line (dtp);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
 
   return 1;
@@ -1405,9 +1409,9 @@  eol_4:
   else if (c != '\n')   
     eat_line (dtp);
 
-  free_line (dtp);
   snprintf (message, MSGLEN, "Bad complex value in item %d of list input",
 	      dtp->u.p.item_count);
+  free_line (dtp);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
 }
 
@@ -1769,7 +1773,7 @@  read_real (st_parameter_dt *dtp, void * dest, int
     {
       dtp->u.p.nml_read_error = 1;
       dtp->u.p.line_buffer_enabled = 1;
-      dtp->u.p.item_count = 0;
+      dtp->u.p.line_buffer_pos = 0;
       return;
     }
 
@@ -1788,9 +1792,9 @@  read_real (st_parameter_dt *dtp, void * dest, int
   else if (c != '\n')
     eat_line (dtp);
 
-  free_line (dtp);
   snprintf (message, MSGLEN, "Bad real number in item %d of list input",
 	      dtp->u.p.item_count);
+  free_line (dtp);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
 }
 
@@ -1805,11 +1809,10 @@  check_type (st_parameter_dt *dtp, bt type, int kin
 
   if (dtp->u.p.saved_type != BT_UNKNOWN && dtp->u.p.saved_type != type)
     {
-      free_line (dtp);
       snprintf (message, MSGLEN, "Read type %s where %s was expected for item %d",
 		  type_name (dtp->u.p.saved_type), type_name (type),
 		  dtp->u.p.item_count);
-
+      free_line (dtp);
       generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
       return 1;
     }
@@ -1820,13 +1823,13 @@  check_type (st_parameter_dt *dtp, bt type, int kin
   if ((type != BT_COMPLEX && dtp->u.p.saved_length != kind)
       || (type == BT_COMPLEX && dtp->u.p.saved_length != kind*2))
     {
-      free_line (dtp);
       snprintf (message, MSGLEN,
 		  "Read kind %d %s where kind %d is required for item %d",
 		  type == BT_COMPLEX ? dtp->u.p.saved_length / 2
 				     : dtp->u.p.saved_length,
 		  type_name (dtp->u.p.saved_type), kind,
 		  dtp->u.p.item_count);
+      free_line (dtp);
       generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
       return 1;
     }