[PATCH] Don't optimize builtins declared always_inline until after inlining (PR middle-end/38454)

Richard Guenther richard.guenther@gmail.com
Tue Dec 9 17:16:00 GMT 2008


On Tue, Dec 9, 2008 at 6:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> If we optimize away builtins that were defined in the headers as inlines
> with always_inline attributes too early (before inlining), they aren't
> ever inlined and thus whatever the headers wanted to do doesn't happen.
>
> On the memcpy-2.c testcase below it causes a regression since the more
> aggressive memcpy folding was added - with -D_FORTIFY_SOURCE{,=2} it
> might overflow a buffer with no compile time warning nor (worse) runtime
> abort.  On memset-1.c it just makes the glibc warning about likely swapped
> memset arguments ineffective.
>
> The following patch avoids folding builtins declared inline with
> always_inline attribute until after inlining.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

!cfun->after_inlining is too agressive (it waits until the final ipa
inliner pass).
It is enough to have always_inline functions inlined which already happens
during early inlining(?).  Also I think the extra checks need a comment _why_
we actually want to delay folding.  There must be also a cheaper way of
testing for always_inline - DECL_DISREGARD_INLINE_LIMITS should be set on these
functions.

Also with delaying this folding - when does it then happen?  Only with the
fold-all-builtins pass or earlier?

Thanks,
Richard.

> 2008-12-09  Jakub Jelinek  <jakub@redhat.com>
>
>        PR middle-end/38454
>        * builtins.c (fold_call_expr): Don't optimize always_inline builtins
>        before inlining.
>        (fold_call_stmt): Likewise.
>        (fold_builtin_call_array): Likewise.  Don't call
>        fold_builtin_varargs for BUILT_IN_MD builtins.
>
>        * gcc.dg/memset-1.c: New test.
>        * gcc.dg/memcpy-2.c: New test.
>
> --- gcc/builtins.c.jj   2008-12-08 20:44:43.000000000 +0100
> +++ gcc/builtins.c      2008-12-09 16:22:55.000000000 +0100
> @@ -10827,6 +10827,14 @@ fold_call_expr (tree exp, bool ignore)
>            return NULL_TREE;
>        }
>
> +      /* Don't fold builtins that have always_inline inlines until after
> +        inlining.  */
> +      if (DECL_DECLARED_INLINE_P (fndecl)
> +         && cfun
> +         && !cfun->after_inlining
> +         && lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl)))
> +       return NULL_TREE;
> +
>       /* FIXME: Don't use a list in this interface.  */
>       if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD)
>          return targetm.fold_builtin (fndecl, CALL_EXPR_ARGS (exp), ignore);
> @@ -10929,6 +10937,13 @@ fold_builtin_call_array (tree type,
>                && DECL_FUNCTION_CODE (fndecl2) == BUILT_IN_VA_ARG_PACK)
>              return build_call_array (type, fn, n, argarray);
>          }
> +       /* Don't fold builtins that have always_inline inlines until after
> +          inlining.  */
> +       if (DECL_DECLARED_INLINE_P (fndecl)
> +           && cfun
> +           && !cfun->after_inlining
> +           && lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl)))
> +         return build_call_array (type, fn, n, argarray);
>         if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD)
>           {
>             tree arglist = NULL_TREE;
> @@ -10937,6 +10952,7 @@ fold_builtin_call_array (tree type,
>             ret = targetm.fold_builtin (fndecl, arglist, false);
>             if (ret)
>               return ret;
> +           return build_call_array (type, fn, n, argarray);
>           }
>         else if (n <= MAX_ARGS_TO_FOLD_BUILTIN)
>           {
> @@ -13645,6 +13661,13 @@ fold_call_stmt (gimple stmt, bool ignore
>     {
>       int nargs = gimple_call_num_args (stmt);
>
> +      /* Don't fold builtins that have always_inline inlines until after
> +        inlining.  */
> +      if (DECL_DECLARED_INLINE_P (fndecl)
> +         && cfun
> +         && !cfun->after_inlining
> +         && lookup_attribute ("always_inline", DECL_ATTRIBUTES (fndecl)))
> +       return NULL_TREE;
>       /* FIXME: Don't use a list in this interface.  */
>       if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD)
>         {
> --- gcc/testsuite/gcc.dg/memset-1.c.jj  2008-12-09 15:51:18.000000000 +0100
> +++ gcc/testsuite/gcc.dg/memset-1.c     2008-12-09 16:35:38.000000000 +0100
> @@ -0,0 +1,28 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +
> +extern void warn_memset_zero_len (void)
> +__attribute__((__warning__ ("memset used with constant zero length parameter;"
> +                           " this could be due to transposed parameters")));
> +
> +extern inline __attribute__((gnu_inline, always_inline, artificial)) void *
> +memset (void *dest, int ch, size_t len)
> +{
> +  if (__builtin_constant_p (len) && len == 0)
> +    {
> +      warn_memset_zero_len (); /* { dg-warning "memset used with constant zero" } */
> +      return dest;
> +    }
> +  return __builtin_memset (dest, ch, len);
> +}
> +
> +char buf[10];
> +
> +int
> +main (void)
> +{
> +  memset (buf, sizeof (buf), 0);
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/memcpy-2.c.jj  2008-12-09 16:37:11.000000000 +0100
> +++ gcc/testsuite/gcc.dg/memcpy-2.c     2008-12-09 16:44:06.000000000 +0100
> @@ -0,0 +1,25 @@
> +/* PR middle-end/38454 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +
> +extern inline __attribute__((gnu_inline, always_inline, artificial)) void *
> +memcpy (void *__restrict dest, const void *__restrict src, size_t len)
> +{
> +  return __builtin___memcpy_chk (dest, /* { dg-warning "will always overflow destination buffer" } */
> +                                src, len, __builtin_object_size (dest, 0));
> +}
> +
> +struct S { char buf[10]; } s;
> +
> +void
> +foo (void)
> +{
> +  char buf[12];
> +  char *p = buf + 4;
> +  struct S *q = (struct S *) p;
> +  memcpy (q, &s, sizeof (s));
> +}
> +
> +/* { dg-final { scan-assembler "__memcpy_chk" } } */
>
>        Jakub
>



More information about the Gcc-patches mailing list