Patchwork Fix ICE on printf optimization with very large format string (PR middle-end/46534)

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 18, 2010, 2:18 p.m.
Message ID <20101118141839.GX29412@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/72096/
State New
Headers show

Comments

Jakub Jelinek - Nov. 18, 2010, 2:18 p.m.
On Thu, Nov 18, 2010 at 01:09:25PM +0100, Richard Guenther wrote:
> On Thu, Nov 18, 2010 at 12:06 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > Hi!
> >
> > For the printf ("...\n") -> puts ("...") optimization we use alloca
> > to copy the string and change it before passing it to build_string_literal.
> > This doesn't work very well if the string is so long that we hit
> > RLIMIT_STACK.
> >
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> 
> As stated in the PR we can avoid the copying completely if by
> adjusting tree.c:build_string to do this modification itself.  No need
> to copy things twice.

So something like this?

Duplicating both build_string and build_string_literal for this special case
would be ugly.

2010-11-18  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/46534
	* builtins.c (fold_builtin_printf): Don't copy and modify string
	before build_string_literal, instead modify what
	build_string_literal returned.

	* gcc.c-torture/compile/pr46534.c: New test.



	Jakub
Richard Guenther - Nov. 18, 2010, 2:42 p.m.
On Thu, Nov 18, 2010 at 3:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Nov 18, 2010 at 01:09:25PM +0100, Richard Guenther wrote:
>> On Thu, Nov 18, 2010 at 12:06 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > Hi!
>> >
>> > For the printf ("...\n") -> puts ("...") optimization we use alloca
>> > to copy the string and change it before passing it to build_string_literal.
>> > This doesn't work very well if the string is so long that we hit
>> > RLIMIT_STACK.
>> >
>> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>> > trunk?
>>
>> As stated in the PR we can avoid the copying completely if by
>> adjusting tree.c:build_string to do this modification itself.  No need
>> to copy things twice.
>
> So something like this?
>
> Duplicating both build_string and build_string_literal for this special case
> would be ugly.
>
> 2010-11-18  Jakub Jelinek  <jakub@redhat.com>
>
>        PR middle-end/46534
>        * builtins.c (fold_builtin_printf): Don't copy and modify string
>        before build_string_literal, instead modify what
>        build_string_literal returned.
>
>        * gcc.c-torture/compile/pr46534.c: New test.
>
> --- gcc/builtins.c.jj   2010-11-18 10:00:20.040373470 +0100
> +++ gcc/builtins.c      2010-11-18 14:48:06.826405229 +0100
> @@ -12892,15 +12892,28 @@ fold_builtin_printf (location_t loc, tre
>        {
>          /* If the string was "string\n", call puts("string").  */
>          size_t len = strlen (str);
> -         if ((unsigned char)str[len - 1] == target_newline)
> +         if ((unsigned char)str[len - 1] == target_newline
> +             && (size_t) (int) len == len
> +             && (int) len > 0)
>            {
> +             char *newstr;
> +             tree offset_node, string_cst;
> +
>              /* Create a NUL-terminated string that's one char shorter
>                 than the original, stripping off the trailing '\n'.  */
> -             char *newstr = XALLOCAVEC (char, len);
> -             memcpy (newstr, str, len - 1);
> -             newstr[len - 1] = 0;
> -
> -             newarg = build_string_literal (len, newstr);
> +             newarg = build_string_literal (len, str);
> +             string_cst = string_constant (newarg, &offset_node);
> +             gcc_checking_assert (string_cst
> +                                  && (TREE_STRING_LENGTH (string_cst)
> +                                      == (int) len)
> +                                  && integer_zerop (offset_node)
> +                                  && (unsigned char)
> +                                     TREE_STRING_POINTER (string_cst)[len - 1]
> +                                     == target_newline);
> +             /* build_string_literal creates a new STRING_CST,
> +                modify it in place to avoid double copying.  */
> +             newstr = CONST_CAST (char *, TREE_STRING_POINTER (string_cst));
> +             newstr[len - 1] = '\0';

Hm, yes.  That works for me.

Richard.

>              if (fn_puts)
>                call = build_call_expr_loc (loc, fn_puts, 1, newarg);
>            }
> --- gcc/testsuite/gcc.c-torture/compile/pr46534.c.jj    2010-11-18 10:07:00.695450656 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/pr46534.c       2010-11-18 10:07:00.695450656 +0100
> @@ -0,0 +1,17 @@
> +/* PR middle-end/46534 */
> +
> +extern int printf (const char *, ...);
> +
> +#define S1 "                    "
> +#define S2 S1 S1 S1 S1 S1 S1 S1 S1 S1 S1
> +#define S3 S2 S2 S2 S2 S2 S2 S2 S2 S2 S2
> +#define S4 S3 S3 S3 S3 S3 S3 S3 S3 S3 S3
> +#define S5 S4 S4 S4 S4 S4 S4 S4 S4 S4 S4
> +#define S6 S5 S5 S5 S5 S5 S5 S5 S5 S5 S5
> +#define S7 S6 S6 S6 S6 S6 S6 S6 S6 S6 S6
> +
> +void
> +foo (void)
> +{
> +  printf (S7 "\n");
> +}
>
>
>        Jakub
>

Patch

--- gcc/builtins.c.jj	2010-11-18 10:00:20.040373470 +0100
+++ gcc/builtins.c	2010-11-18 14:48:06.826405229 +0100
@@ -12892,15 +12892,28 @@  fold_builtin_printf (location_t loc, tre
 	{
 	  /* If the string was "string\n", call puts("string").  */
 	  size_t len = strlen (str);
-	  if ((unsigned char)str[len - 1] == target_newline)
+	  if ((unsigned char)str[len - 1] == target_newline
+	      && (size_t) (int) len == len
+	      && (int) len > 0)
 	    {
+	      char *newstr;
+	      tree offset_node, string_cst;
+
 	      /* Create a NUL-terminated string that's one char shorter
 		 than the original, stripping off the trailing '\n'.  */
-	      char *newstr = XALLOCAVEC (char, len);
-	      memcpy (newstr, str, len - 1);
-	      newstr[len - 1] = 0;
-
-	      newarg = build_string_literal (len, newstr);
+	      newarg = build_string_literal (len, str);
+	      string_cst = string_constant (newarg, &offset_node);
+	      gcc_checking_assert (string_cst
+				   && (TREE_STRING_LENGTH (string_cst)
+				       == (int) len)
+				   && integer_zerop (offset_node)
+				   && (unsigned char)
+				      TREE_STRING_POINTER (string_cst)[len - 1]
+				      == target_newline);
+	      /* build_string_literal creates a new STRING_CST,
+		 modify it in place to avoid double copying.  */
+	      newstr = CONST_CAST (char *, TREE_STRING_POINTER (string_cst));
+	      newstr[len - 1] = '\0';
 	      if (fn_puts)
 		call = build_call_expr_loc (loc, fn_puts, 1, newarg);
 	    }
--- gcc/testsuite/gcc.c-torture/compile/pr46534.c.jj	2010-11-18 10:07:00.695450656 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr46534.c	2010-11-18 10:07:00.695450656 +0100
@@ -0,0 +1,17 @@ 
+/* PR middle-end/46534 */
+
+extern int printf (const char *, ...);
+
+#define S1 "                    "
+#define S2 S1 S1 S1 S1 S1 S1 S1 S1 S1 S1
+#define S3 S2 S2 S2 S2 S2 S2 S2 S2 S2 S2
+#define S4 S3 S3 S3 S3 S3 S3 S3 S3 S3 S3
+#define S5 S4 S4 S4 S4 S4 S4 S4 S4 S4 S4
+#define S6 S5 S5 S5 S5 S5 S5 S5 S5 S5 S5
+#define S7 S6 S6 S6 S6 S6 S6 S6 S6 S6 S6
+
+void
+foo (void)
+{
+  printf (S7 "\n");
+}