[RFC PATCH] __builtin_va_arg_pack ()

[Richard Guenther]

On 8/31/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Aug 31, 2007 at 11:14:49AM +0200, Richard Guenther wrote:
> > I think the idea is sound.  Though for instrumenting the standard functions
> > like printf couldn't you dispatch to the vprintf variants instead?  I
> > see we'd need
> > to force the compiler to not inline that call then which works by re-declaring
> > it like
> >
> > extern inline __attribute__((__gnu_inline__)) void foo(void) {}
> >
> > extern inline __attribute__((__gnu_inline__)) void bar(void)
> > {
> >   extern __attribute__((noinline)) void foo(void);
> >
> >   foo();
> > }
> >
> > but unfortunately you get an extra warning for from that and further calls
> > to foo() outside of bar() are not inlined anymore.  Leaving the only
> > other possibility to add a #pragma to mark a specific call as not-to-inline.
>
> The -D_FORTIFY_SOURCE checking relies on inlining the wrappers.
> E.g. for sprintf we currently have:
> #define sprintf(str, ...) \
>   __builtin___sprintf_chk (str, __USE_FORTIFY_LEVEL - 1, __bos (str), \
>                            __VA_ARGS__)
> which can't be used in C++.  With the proposed __builtin_va_arg_pack ()
> it can be implemented as
> __extern_always_inline int
> sprintf (char *__restrict __s, __const char *__restrict __format, ...)
> {
>   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1, __bos (__s),
>                                   __format, __builtin_va_arg_pack ());
> }
> where
> #define __always_inline __inline __attribute__ ((__always_inline__))
> #define __extern_always_inline extern __always_inline __attribute__ ((__gnu_inline__))
> #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>
> If you instead want to call vsprintf, it couldn't be inlined:
> static inline int
> sprintf (char *__restrict __s, __const char *__restrict __format, ...)
> {
>   va_list __ap;
>   int __ret;
>   va_start (__ap, __format);
>   __ret = __builtin___vsprintf_chk (__s, __USE_FORTIFY_LEVEL - 1, __bos (__s),
>                                     __format, __ap);
>   va_end (__ap);
>   return __ret;
> }
> (at least, hacking up gcc so that it could inline this would be terribly
> difficult and extremely arch dependent), which unfortunately means
> that __builtin_object_size (__s, __USE_FORTIFY_LEVEL - 1) will
> always return -1UL - at least until we have ipa object size pass and cloning
> based on that (but the cloning would need to create a special out of
> line function for each different value of __bos, or figure it adds an
> artificial argument.  Even if it does, it is another hop the call to
> sprintf has to go through, so at least a few cycles wasted).
> But __bos (__s) == -1UL means no buffer overflow checking.
>
> >
> > > +                 new_call = build_call_array (TREE_TYPE (call),
> > > +                                              CALL_EXPR_FN (call),
> > > +                                              nargs + call_expr_nargs (call),
> > > +                                              argarray);
> > > +                 CALL_EXPR_STATIC_CHAIN (new_call)
> > > +                   = CALL_EXPR_STATIC_CHAIN (call);
> > > +                 CALL_EXPR_TAILCALL (new_call) = CALL_EXPR_TAILCALL (call);
> > > +                 CALL_EXPR_RETURN_SLOT_OPT (new_call)
> > > +                   = CALL_EXPR_RETURN_SLOT_OPT (call);
> >
> > in tree.h I also find CALL_FROM_THUNK_P, CALL_CANNOT_INLINE_P and
> > TREE_NOTHROW.
>
> You are right, fixed.  I wonder if there is a better method for copying
> the flags one by one, which is error prone.  We want to copy all the
> flags, except CALL_EXPR_VA_ARG_PACK and guess ann shouldn't be copied over
> either.  But doing
> CALL_EXPR_CHECK (new_call)->base = CALL_EXPR_CHECK (call)->base;
> sounds fragile too.

Yeah.  I think we should have explicit copying functions for all tree
codes ;)  Or
as a compromise a function that walks based on the tree code the
struct hierarchy
and copies all fields (or all flags only).  That way we would have one
central place
to update.

> BTW, OT, why isn't
> /* Used to mark a CALL_EXPR as not suitable for inlining.  */
> #define CALL_CANNOT_INLINE_P(NODE) ((NODE)->base.static_flag)
> using CALL_EXPR_CHECK?

I missed adding it.

> Here is an updated patch:

That looks good.  But I don't think I can approve it.  Still I'd like
to have it in
in 4.3 if it works out to support fortifying for C++.  So, Ian/Diego
can you have
a look here please?

Thanks!
Richard.

> 2007-08-31  Jakub Jelinek  <jakub@redhat.com>
>
>         * builtins.def (BUILT_IN_VA_ARG_PACK): New built-in.
>         * tree.h (CALL_EXPR_VA_ARG_PACK): Define.
>         * tree-inline.h (copy_body_data): Add call_expr field.
>         * tree-inline.c (expand_call_inline): Initialize call_expr.
>         (copy_bb): Append anonymous inline fn arguments to arguments
>         when inlining a CALL_EXPR_VA_ARG_PACK call.
>         * builtins.c (expand_builtin): Issue an error if
>         BUILT_IN_VA_ARG_PACK is seen during expand.
>         (fold_call_expr, fold_builtin_call_array): Don't fold
>         CALL_EXPR_VA_ARG_PACK CALL_EXPRs or calls with
>         __builtin_va_arg_pack () call as last argument.
>         * gimplify.c (gimplify_call_expr): If last argument to a vararg
>         function is __builtin_va_arg_pack (), decrease number of call
>         arguments and instead set CALL_EXPR_VA_ARG_PACK on the CALL_EXPR.
>         * expr.c (expand_expr_real_1): Issue an error if
>         CALL_EXPR_VA_ARG_PACK CALL_EXPR is seen during expand.
>         * tree-pretty-print.c (dump_generic_node): Handle printing
>         CALL_EXPR_VA_ARG_PACK bit on CALL_EXPRs.
>         * doc/extend.texi (__builtin_va_arg_pack): Document.
>
>         * gcc.c-torture/execute/va-arg-pack-1.c: New test.
>         * gcc.dg/va-arg-pack-1.c: New test.