Re: [PATCH, i386, Pointer Bounds Checker 18/x] Expand instrumented builtin function calls

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

On Mon, Jun 2, 2014 at 4:51 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hi,
>
> This patch adds support for normal builtin function calls (target ones are not instrumented).  The basic idea of the patch is to make call expr copy with no bounds and expand it instead.  If expr is going to be emitted as a function call then original instrumented expr takes place.  Handle string function separately because they are of high interest for the checker.

It seems to be this should be user-controllable (always expand builtins with
bounds as calls, or not), rather than deciding what is of interest yourself.

From a high-level perspective the expansion path doing inline expansion
should be separated from that expanding as a call (or expanding as a
different call).  I don't like that building of yet another GENERIC call expr
in this code ... it goes very much backwards of the idea that we should
expand from the GIMPLE representation.  You are making such transition
even harder.

Can't you do that frobbing from cfgexpand.c:expand_call_stmt instead (please!)?

Thanks,
Richard.

> Bootstrapped and tested on linux-x86_64.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-06-02  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * builtins.c: Include rtl-chkp.h, tree-chkp.h.
>         (expand_builtin_mempcpy_args): Add orig exp as argument.
>         Support BUILT_IN_CHKP_MEMPCPY_NOBND_NOCHK.
>         (expand_builtin_mempcpy): Adjust expand_builtin_mempcpy_args call.
>         (expand_builtin_stpcpy): Likewise.
>         (expand_builtin_memset_args): Support BUILT_IN_CHKP_MEMSET_NOBND_NOCHK.
>         (std_expand_builtin_va_start): Initialize bounds for va_list.
>         (expand_builtin): Support instrumented calls.
>
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 7357124..c0140e1 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -59,6 +59,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "builtins.h"
>  #include "ubsan.h"
>  #include "cilk.h"
> +#include "tree-chkp.h"
> +#include "rtl-chkp.h"
>
>
>  static tree do_mpc_arg1 (tree, tree, int (*)(mpc_ptr, mpc_srcptr, mpc_rnd_t));
> @@ -124,7 +126,7 @@ static rtx builtin_memcpy_read_str (void *, HOST_WIDE_INT, enum machine_mode);
>  static rtx expand_builtin_memcpy (tree, rtx);
>  static rtx expand_builtin_mempcpy (tree, rtx, enum machine_mode);
>  static rtx expand_builtin_mempcpy_args (tree, tree, tree, rtx,
> -                                       enum machine_mode, int);
> +                                       enum machine_mode, int, tree);
>  static rtx expand_builtin_strcpy (tree, rtx);
>  static rtx expand_builtin_strcpy_args (tree, tree, rtx);
>  static rtx expand_builtin_stpcpy (tree, rtx, enum machine_mode);
> @@ -3284,7 +3286,8 @@ expand_builtin_mempcpy (tree exp, rtx target, enum machine_mode mode)
>        tree src = CALL_EXPR_ARG (exp, 1);
>        tree len = CALL_EXPR_ARG (exp, 2);
>        return expand_builtin_mempcpy_args (dest, src, len,
> -                                         target, mode, /*endp=*/ 1);
> +                                         target, mode, /*endp=*/ 1,
> +                                         exp);
>      }
>  }
>
> @@ -3296,10 +3299,23 @@ expand_builtin_mempcpy (tree exp, rtx target, enum machine_mode mode)
>
>  static rtx
>  expand_builtin_mempcpy_args (tree dest, tree src, tree len,
> -                            rtx target, enum machine_mode mode, int endp)
> +                            rtx target, enum machine_mode mode, int endp,
> +                            tree orig_exp)
>  {
> +  tree fndecl = get_callee_fndecl (orig_exp);
> +
>      /* If return value is ignored, transform mempcpy into memcpy.  */
> -  if (target == const0_rtx && builtin_decl_implicit_p (BUILT_IN_MEMCPY))
> +  if (target == const0_rtx
> +      && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_CHKP_MEMPCPY_NOBND_NOCHK
> +      && builtin_decl_implicit_p (BUILT_IN_CHKP_MEMCPY_NOBND_NOCHK))
> +    {
> +      tree fn = builtin_decl_implicit (BUILT_IN_CHKP_MEMCPY_NOBND_NOCHK);
> +      tree result = build_call_nofold_loc (UNKNOWN_LOCATION, fn, 3,
> +                                          dest, src, len);
> +      return expand_expr (result, target, mode, EXPAND_NORMAL);
> +    }
> +  else if (target == const0_rtx
> +          && builtin_decl_implicit_p (BUILT_IN_MEMCPY))
>      {
>        tree fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
>        tree result = build_call_nofold_loc (UNKNOWN_LOCATION, fn, 3,
> @@ -3484,7 +3500,8 @@ expand_builtin_stpcpy (tree exp, rtx target, enum machine_mode mode)
>
>        lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
>        ret = expand_builtin_mempcpy_args (dst, src, lenp1,
> -                                        target, mode, /*endp=*/2);
> +                                        target, mode, /*endp=*/2,
> +                                        exp);
>
>        if (ret)
>         return ret;
> @@ -3778,7 +3795,8 @@ expand_builtin_memset_args (tree dest, tree val, tree len,
>   do_libcall:
>    fndecl = get_callee_fndecl (orig_exp);
>    fcode = DECL_FUNCTION_CODE (fndecl);
> -  if (fcode == BUILT_IN_MEMSET)
> +  if (fcode == BUILT_IN_MEMSET
> +      || fcode == BUILT_IN_CHKP_MEMSET_NOBND_NOCHK)
>      fn = build_call_nofold_loc (EXPR_LOCATION (orig_exp), fndecl, 3,
>                                 dest, val, len);
>    else if (fcode == BUILT_IN_BZERO)
> @@ -4330,6 +4348,13 @@ std_expand_builtin_va_start (tree valist, rtx nextarg)
>  {
>    rtx va_r = expand_expr (valist, NULL_RTX, VOIDmode, EXPAND_WRITE);
>    convert_move (va_r, nextarg, 0);
> +
> +  /* We do not have any valid bounds for the pointer, so
> +     just store zero bounds for it.  */
> +  if (chkp_function_instrumented_p (current_function_decl))
> +    chkp_expand_bounds_reset_for_mem (valist,
> +                                     make_tree (TREE_TYPE (valist),
> +                                                nextarg));
>  }
>
>  /* Expand EXP, a call to __builtin_va_start.  */
> @@ -5793,6 +5818,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>    enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
>    enum machine_mode target_mode = TYPE_MODE (TREE_TYPE (exp));
>    int flags;
> +  tree orig_exp = exp;
>
>    if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD)
>      return targetm.expand_builtin (exp, target, subtarget, mode, ignore);
> @@ -5856,6 +5882,41 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>         }
>      }
>
> +  /* Currently none of builtin expand functions works with bounds.
> +     To avoid modification of all expanders we just make a new call
> +     expression without bound args.  The original expression is used
> +     in case we expand builtin as a call.  */
> +  if (CALL_WITH_BOUNDS_P (exp))
> +    {
> +      int new_arg_no = 0;
> +      tree new_call;
> +      tree arg;
> +      call_expr_arg_iterator iter;
> +      tree *new_args = XALLOCAVEC (tree, call_expr_nargs (exp));
> +
> +      FOR_EACH_CALL_EXPR_ARG (arg, iter, exp)
> +       if (!POINTER_BOUNDS_P (arg))
> +         new_args[new_arg_no++] = arg;
> +
> +      if (new_arg_no > 0)
> +       {
> +         new_call = build_call_array (TREE_TYPE (exp), fndecl, new_arg_no, new_args);
> +         CALL_EXPR_STATIC_CHAIN (new_call) = CALL_EXPR_STATIC_CHAIN (exp);
> +         CALL_EXPR_FN (new_call) = CALL_EXPR_FN (exp);
> +         TREE_SIDE_EFFECTS (new_call) = TREE_SIDE_EFFECTS (exp);
> +         TREE_NOTHROW (new_call) = TREE_NOTHROW (exp);
> +         CALL_EXPR_TAILCALL (new_call) = CALL_EXPR_TAILCALL (exp);
> +         CALL_EXPR_RETURN_SLOT_OPT (new_call) = CALL_EXPR_RETURN_SLOT_OPT (exp);
> +         CALL_ALLOCA_FOR_VAR_P (new_call) = CALL_ALLOCA_FOR_VAR_P (exp);
> +         CALL_FROM_THUNK_P (new_call) = CALL_FROM_THUNK_P (exp);
> +         CALL_EXPR_VA_ARG_PACK (new_call) = CALL_EXPR_VA_ARG_PACK (exp);
> +         SET_EXPR_LOCATION (new_call, EXPR_LOCATION (exp));
> +         TREE_SET_BLOCK (new_call, TREE_BLOCK (exp));
> +
> +         exp = new_call;
> +        }
> +    }
> +
>    switch (fcode)
>      {
>      CASE_FLT_FN (BUILT_IN_FABS):
> @@ -6146,60 +6207,116 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>        break;
>
>      case BUILT_IN_STRLEN:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_strlen (exp, target, target_mode);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_STRCPY:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_strcpy (exp, target);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_STRNCPY:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_strncpy (exp, target);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_STPCPY:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_stpcpy (exp, target, mode);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_MEMCPY:
> +    case BUILT_IN_CHKP_MEMCPY_NOBND_NOCHK:
> +      if (CALL_WITH_BOUNDS_P (orig_exp)
> +         && fcode == BUILT_IN_MEMCPY)
> +       break;
>        target = expand_builtin_memcpy (exp, target);
>        if (target)
> -       return target;
> +       {
> +         /* We need to set returned bounds for instrumented
> +            calls.  */
> +         if (CALL_WITH_BOUNDS_P (orig_exp))
> +           {
> +             rtx bnd = force_reg (BNDmode,
> +                                  expand_normal (CALL_EXPR_ARG (orig_exp, 1)));
> +             target = chkp_join_splitted_slot (target, bnd);
> +           }
> +         return target;
> +       }
>        break;
>
>      case BUILT_IN_MEMPCPY:
> +      case BUILT_IN_CHKP_MEMPCPY_NOBND_NOCHK:
> +       if (CALL_WITH_BOUNDS_P (orig_exp)
> +           && fcode == BUILT_IN_MEMPCPY)
> +       break;
>        target = expand_builtin_mempcpy (exp, target, mode);
>        if (target)
> -       return target;
> +       {
> +         /* We need to set returned bounds for instrumented
> +            calls.  */
> +         if (CALL_WITH_BOUNDS_P (orig_exp))
> +           {
> +             rtx bnd = force_reg (BNDmode,
> +                                  expand_normal (CALL_EXPR_ARG (orig_exp, 1)));
> +             target = chkp_join_splitted_slot (target, bnd);
> +           }
> +         return target;
> +       }
>        break;
>
>      case BUILT_IN_MEMSET:
> +    case BUILT_IN_CHKP_MEMSET_NOBND_NOCHK:
> +      if (CALL_WITH_BOUNDS_P (orig_exp)
> +         && fcode == BUILT_IN_MEMSET)
> +       break;
>        target = expand_builtin_memset (exp, target, mode);
>        if (target)
> -       return target;
> +       {
> +         /* We need to set returned bounds for instrumented
> +            calls.  */
> +         if (CALL_WITH_BOUNDS_P (orig_exp))
> +           {
> +             rtx bnd = force_reg (BNDmode,
> +                                  expand_normal (CALL_EXPR_ARG (orig_exp, 1)));
> +             target = chkp_join_splitted_slot (target, bnd);
> +           }
> +         return target;
> +       }
>        break;
>
>      case BUILT_IN_BZERO:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_bzero (exp);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_STRCMP:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_strcmp (exp, target);
>        if (target)
>         return target;
>        break;
>
>      case BUILT_IN_STRNCMP:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_strncmp (exp, target, mode);
>        if (target)
>         return target;
> @@ -6207,6 +6324,8 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>
>      case BUILT_IN_BCMP:
>      case BUILT_IN_MEMCMP:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_memcmp (exp, target, mode);
>        if (target)
>         return target;
> @@ -6588,14 +6707,17 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>
>         /* If this is turned into an external library call, the weak parameter
>            must be dropped to match the expected parameter list.  */
> -       nargs = call_expr_nargs (exp);
> +       nargs = call_expr_nargs (orig_exp);
>         vec_alloc (vec, nargs - 1);
> -       for (z = 0; z < 3; z++)
> -         vec->quick_push (CALL_EXPR_ARG (exp, z));
> -       /* Skip the boolean weak parameter.  */
> -       for (z = 4; z < 6; z++)
> -         vec->quick_push (CALL_EXPR_ARG (exp, z));
> -       exp = build_call_vec (TREE_TYPE (exp), CALL_EXPR_FN (exp), vec);
> +       for (z = 0; z < nargs; z++)
> +         /* Skip the boolean weak parameter.  */
> +         if ((!CALL_WITH_BOUNDS_P (orig_exp) && z == 3)
> +             || (CALL_WITH_BOUNDS_P (orig_exp) && z == 5))
> +           continue;
> +         else
> +           vec->quick_push (CALL_EXPR_ARG (orig_exp, z));
> +       orig_exp = build_call_vec (TREE_TYPE (orig_exp),
> +                                  CALL_EXPR_FN (orig_exp), vec);
>         break;
>        }
>
> @@ -6819,6 +6941,8 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>      case BUILT_IN_MEMPCPY_CHK:
>      case BUILT_IN_MEMMOVE_CHK:
>      case BUILT_IN_MEMSET_CHK:
> +      if (CALL_WITH_BOUNDS_P (orig_exp))
> +       break;
>        target = expand_builtin_memory_chk (exp, target, mode, fcode);
>        if (target)
>         return target;
> @@ -6873,7 +6997,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>      case BUILT_IN_CHKP_GET_PTR_UBOUND:
>        /* We allow user CHKP builtins if Pointer Bounds
>          Checker is off.  */
> -      if (!flag_check_pointer_bounds)
> +      if (!chkp_function_instrumented_p (current_function_decl))
>         {
>           if (fcode == BUILT_IN_CHKP_SET_PTR_BOUNDS
>               || fcode == BUILT_IN_CHKP_NARROW_PTR_BOUNDS
> @@ -6911,7 +7035,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode,
>
>    /* The switch statement above can drop through to cause the function
>       to be called normally.  */
> -  return expand_call (exp, target, ignore);
> +  return expand_call (orig_exp, target, ignore);
>  }
>
>  /* Determine whether a tree node represents a call to a built-in