diff mbox

[Fortran] PR56737 - Fixing a bug in the I/O format cache handling

Message ID 515419B1.5060102@net-b.de
State New
Headers show

Commit Message

Tobias Burnus March 28, 2013, 10:21 a.m. UTC 
Tobias Burnus wrote:
> b) To copy the format string
>
> The attached patch does the latter. The current hashing algorithm 
> avoids hash collisions by checking whether the value is exactly the 
> same - and the value is given by the format string. Thus, instead of 
> copying the string when storing the format in the cache, the patch 
> copies it now before calling parse_format_list.

Re-reading what Jerry wrote, I realized that the current code disables 
format caching for strings (but not for Holleriths). With my patch, 
that's no longer required. Attached is a missed-optimization patch.

> Bootstrapped and regtested on x86-64-gnu-linux.
> OK for the trunk and the 4.6/4.7/4.8 branches?

(Or should the follow-up patch only applied to the trunk?)


BTW: Without the follow up patch, the following is a memory leak as 
parse_format_list might set format_cache_ok to false:
>     if (fmt->error)
>       {
>         format_error (dtp, NULL, fmt->error);
> +      if (format_cache_ok)
> +	free (dtp->format);

Thus, if the follow-up patch is not backported, it should be changed to 
"!is_internal_unit (dtp)", which is the initial condition for 
format_cache_ok.

Tobias

Comments

Jerry DeLisle March 29, 2013, 1:29 a.m. UTC | #1 
On 03/28/2013 03:21 AM, Tobias Burnus wrote:
> Tobias Burnus wrote:
>> b) To copy the format string
>>
>> The attached patch does the latter. The current hashing algorithm avoids hash
>> collisions by checking whether the value is exactly the same - and the value
>> is given by the format string. Thus, instead of copying the string when
>> storing the format in the cache, the patch copies it now before calling
>> parse_format_list.
> 
> Re-reading what Jerry wrote, I realized that the current code disables format
> caching for strings (but not for Holleriths). With my patch, that's no longer
> required. Attached is a missed-optimization patch.
> 
>> Bootstrapped and regtested on x86-64-gnu-linux.
>> OK for the trunk and the 4.6/4.7/4.8 branches?
> 
> (Or should the follow-up patch only applied to the trunk?)
> 

I would treat all as part of one patch fixing the original issue which was a
known TODO:

Please goto trunk first and give it some settling time.  If all is OK, I would
go ahead and backport. It does fix a regression.

Jerry
diff mbox

Patch

2012-03-28  Tobias Burnus  <burnus@net-b.de>

	PR fortran/56737
	* io/format.c (parse_format_list): Also cache FMT_STRING.
	(parse_format): Update call.

diff --git a/libgfortran/io/format.c b/libgfortran/io/format.c
index db95e49..d5a3548 100644
--- a/libgfortran/io/format.c
+++ b/libgfortran/io/format.c
@@ -586,16 +586,15 @@  format_lex (format_data *fmt)
  * parenthesis node which contains the rest of the list. */
 
 static fnode *
-parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
+parse_format_list (st_parameter_dt *dtp, bool *seen_dd)
 {
   fnode *head, *tail;
   format_token t, u, t2;
   int repeat;
   format_data *fmt = dtp->u.p.fmt;
-  bool saveit, seen_data_desc = false;
+  bool seen_data_desc = false;
 
   head = tail = NULL;
-  saveit = *save_ok;
 
   /* Get the next format item */
  format_item:
@@ -612,7 +611,7 @@  parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
 	}
       get_fnode (fmt, &head, &tail, FMT_LPAREN);
       tail->repeat = -2;  /* Signifies unlimited format.  */
-      tail->u.child = parse_format_list (dtp, &saveit, &seen_data_desc);
+      tail->u.child = parse_format_list (dtp, &seen_data_desc);
       if (fmt->error != NULL)
 	goto finished;
       if (!seen_data_desc)
@@ -631,7 +630,7 @@  parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
 	case FMT_LPAREN:
 	  get_fnode (fmt, &head, &tail, FMT_LPAREN);
 	  tail->repeat = repeat;
-	  tail->u.child = parse_format_list (dtp, &saveit, &seen_data_desc);
+	  tail->u.child = parse_format_list (dtp, &seen_data_desc);
 	  *seen_dd = seen_data_desc;
 	  if (fmt->error != NULL)
 	    goto finished;
@@ -659,7 +658,7 @@  parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
     case FMT_LPAREN:
       get_fnode (fmt, &head, &tail, FMT_LPAREN);
       tail->repeat = 1;
-      tail->u.child = parse_format_list (dtp, &saveit, &seen_data_desc);
+      tail->u.child = parse_format_list (dtp, &seen_data_desc);
       *seen_dd = seen_data_desc;
       if (fmt->error != NULL)
 	goto finished;
@@ -723,8 +722,6 @@  parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
       goto between_desc;
 
     case FMT_STRING:
-      /* TODO: Find out why it is necessary to turn off format caching.  */
-      saveit = false;
       get_fnode (fmt, &head, &tail, FMT_STRING);
       tail->u.string.p = fmt->string;
       tail->u.string.length = fmt->value;
@@ -1104,8 +1101,6 @@  parse_format_list (st_parameter_dt *dtp, bool *save_ok, bool *seen_dd)
 
  finished:
 
-  *save_ok = saveit;
-  
   return head;
 }
 
@@ -1255,8 +1250,7 @@  parse_format (st_parameter_dt *dtp)
   fmt->avail++;
 
   if (format_lex (fmt) == FMT_LPAREN)
-    fmt->array.array[0].u.child = parse_format_list (dtp, &format_cache_ok,
-						     &seen_data_desc);
+    fmt->array.array[0].u.child = parse_format_list (dtp, &seen_data_desc);
   else
     fmt->error = "Missing initial left parenthesis in format";