[PATCH] Fix alter_output_for_subst_insn (PR other/77421)

[PATCH] Fix alter_output_for_subst_insn (PR other/77421)

[Jakub Jelinek]

Hi!

The two ports that use define_subst (ix86 and visium) don't do anything in
this function, other than returning early - return insn_out, so all I could
do is look at the intent.
The *insn_out == '*' || *insn_out != '@' got flagged by some tool, the
"*insn_out == '*' || " part is unnecessary, since '*' != '@'.  I guess the
reason for it being there is that template starting with * is C code (which
this function has nothing to do for), template starting with @ is what the
function wants to do something about and other template strings it wants to
ignore too.

But, when I got to this function, I found various weirdo formatting and
other issues, the most important is that it leaks memory and in my
understanding allocates that buffer completely uselessly, as it is pretty
much a strdup of the insn_out after skipping initial spaces (and all @,
which is weird, IMNSHO the only @ that it should skip is the very first
one), except that '\0' isn't there and the length is remembered.  But, we
don't change it at all, so we can as well use the original insn_out.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-09-02  Jakub Jelinek  <jakub@redhat.com>

	PR other/77421
	* gensupport.c (alter_output_for_subst_insn): Remove redundant
	*insn_out == '*' test.  Don't copy unnecessary to yet another
	memory buffer, and don't leak it.

--- gcc/gensupport.c.jj	2016-08-12 17:33:38.000000000 +0200
+++ gcc/gensupport.c	2016-09-02 14:55:01.217182312 +0200
@@ -1632,33 +1632,30 @@ duplicate_each_alternative (const char *
 static const char *
 alter_output_for_subst_insn (rtx insn, int alt)
 {
-  const char *insn_out, *sp ;
-  char *old_out, *new_out, *cp;
-  int i, j, new_len;
+  const char *insn_out, *old_out;
+  char *new_out, *cp;
+  size_t old_len, new_len;
+  int j;
 
   insn_out = XTMPL (insn, 3);
 
-  if (alt < 2 || *insn_out == '*' || *insn_out != '@')
+  if (alt < 2 || *insn_out != '@')
     return insn_out;
 
-  old_out = XNEWVEC (char, strlen (insn_out)),
-  sp = insn_out;
+  old_out = insn_out + 1;
+  while (ISSPACE (*old_out))
+    old_out++;
+  old_len = strlen (old_out);
 
-  while (ISSPACE (*sp) || *sp == '@')
-    sp++;
-
-  for (i = 0; *sp;)
-    old_out[i++] = *sp++;
-
-  new_len = alt * (i + 1) + 1;
+  new_len = alt * (old_len + 1) + 1;
 
   new_out = XNEWVEC (char, new_len);
   new_out[0] = '@';
 
-  for (j = 0, cp = new_out + 1; j < alt; j++, cp += i + 1)
+  for (j = 0, cp = new_out + 1; j < alt; j++, cp += old_len + 1)
     {
-      memcpy (cp, old_out, i);
-      *(cp+i) = (j == alt - 1) ? '\0' : '\n';
+      memcpy (cp, old_out, old_len);
+      cp[old_len] = (j == alt - 1) ? '\0' : '\n';
     }
 
   return new_out;

	Jakub

Re: [PATCH] Fix alter_output_for_subst_insn (PR other/77421)

[Richard Biener]

On Fri, 2 Sep 2016, Jakub Jelinek wrote:

> Hi!
> 
> The two ports that use define_subst (ix86 and visium) don't do anything in
> this function, other than returning early - return insn_out, so all I could
> do is look at the intent.
> The *insn_out == '*' || *insn_out != '@' got flagged by some tool, the
> "*insn_out == '*' || " part is unnecessary, since '*' != '@'.  I guess the
> reason for it being there is that template starting with * is C code (which
> this function has nothing to do for), template starting with @ is what the
> function wants to do something about and other template strings it wants to
> ignore too.
> 
> But, when I got to this function, I found various weirdo formatting and
> other issues, the most important is that it leaks memory and in my
> understanding allocates that buffer completely uselessly, as it is pretty
> much a strdup of the insn_out after skipping initial spaces (and all @,
> which is weird, IMNSHO the only @ that it should skip is the very first
> one), except that '\0' isn't there and the length is remembered.  But, we
> don't change it at all, so we can as well use the original insn_out.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2016-09-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR other/77421
> 	* gensupport.c (alter_output_for_subst_insn): Remove redundant
> 	*insn_out == '*' test.  Don't copy unnecessary to yet another
> 	memory buffer, and don't leak it.
> 
> --- gcc/gensupport.c.jj	2016-08-12 17:33:38.000000000 +0200
> +++ gcc/gensupport.c	2016-09-02 14:55:01.217182312 +0200
> @@ -1632,33 +1632,30 @@ duplicate_each_alternative (const char *
>  static const char *
>  alter_output_for_subst_insn (rtx insn, int alt)
>  {
> -  const char *insn_out, *sp ;
> -  char *old_out, *new_out, *cp;
> -  int i, j, new_len;
> +  const char *insn_out, *old_out;
> +  char *new_out, *cp;
> +  size_t old_len, new_len;
> +  int j;
>  
>    insn_out = XTMPL (insn, 3);
>  
> -  if (alt < 2 || *insn_out == '*' || *insn_out != '@')
> +  if (alt < 2 || *insn_out != '@')
>      return insn_out;
>  
> -  old_out = XNEWVEC (char, strlen (insn_out)),
> -  sp = insn_out;
> +  old_out = insn_out + 1;
> +  while (ISSPACE (*old_out))
> +    old_out++;
> +  old_len = strlen (old_out);
>  
> -  while (ISSPACE (*sp) || *sp == '@')
> -    sp++;
> -
> -  for (i = 0; *sp;)
> -    old_out[i++] = *sp++;
> -
> -  new_len = alt * (i + 1) + 1;
> +  new_len = alt * (old_len + 1) + 1;
>  
>    new_out = XNEWVEC (char, new_len);
>    new_out[0] = '@';
>  
> -  for (j = 0, cp = new_out + 1; j < alt; j++, cp += i + 1)
> +  for (j = 0, cp = new_out + 1; j < alt; j++, cp += old_len + 1)
>      {
> -      memcpy (cp, old_out, i);
> -      *(cp+i) = (j == alt - 1) ? '\0' : '\n';
> +      memcpy (cp, old_out, old_len);
> +      cp[old_len] = (j == alt - 1) ? '\0' : '\n';
>      }
>  
>    return new_out;
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)