Re: [PATCH] Fix ICE on printf optimization with very large format string (PR middle-end/46534)

[Richard Guenther <richard dot guenther at gmail dot com>]

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
>