[PATCH, i386, Pointer Bounds Checker 33/x] MPX ABI

Uros Bizjak ubizjak@gmail.com
Sun Sep 21 12:35:00 GMT 2014


On Fri, Sep 19, 2014 at 9:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:

> Here is an updated version of this patch.
>
> Thanks,
> Ilya
> --
> 2014-09-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * config/i386/i386.c (ix86_option_override_internal): Do not
>         support x32 with MPX.
>         is not available.
>         (init_cumulative_args): Init stdarg, bnd_regno, bnds_in_bt
>         and force_bnd_pass.
>         (function_arg_advance_32): Return number of used integer
>         registers.
>         (function_arg_advance_64): Likewise.
>         (function_arg_advance_ms_64): Likewise.
>         (ix86_function_arg_advance): Handle pointer bounds.
>         (ix86_function_arg): Likewise.
>         (ix86_function_value_regno_p): Mark fisrt bounds registers as
>         possible function value.
>         (ix86_function_value_1): Handle pointer bounds type/mode
>         (ix86_return_in_memory): Likewise.
>         (ix86_print_operand): Analyse insn to decide abounf"bnd" prefix.
>         (ix86_expand_call): Generate returned bounds.
>         (ix86_bnd_prefixed_insn_p): Check if we have instrumented call
>         or function.
>         * config/i386/i386.h (ix86_args): Add bnd_regno, bnds_in_bt,
>         force_bnd_pass and stdarg fields.
>         * config/i386/i386.md (UNSPEC_BNDRET): New.
>         (*call_value): Add returned bounds.
>         (*sibcall_value): Likewise.
>         (*call_value_rex64_ms_sysv): Likewise.
>         (*call_value_pop): Likewise.
>         (*sibcall_value_pop): Likewise.
>         * config/i386/predicates.md (call_rex64_ms_sysv_operation): Adjust
>         to changed call patterns.
>
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 4a58190..da7ffa8 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -3712,6 +3712,9 @@ ix86_option_override_internal (bool main_args_p,
>    if (TARGET_X32 && (opts->x_ix86_isa_flags & OPTION_MASK_ISA_MPX))
>      error ("Intel MPX does not support x32");
>
> +  if (TARGET_X32 && (ix86_isa_flags & OPTION_MASK_ISA_MPX))
> +    error ("Intel MPX does not support x32");
> +
>    if (!strcmp (opts->x_ix86_arch_string, "generic"))
>      error ("generic CPU can be used only for %stune=%s %s",
>            prefix, suffix, sw);
> @@ -6198,10 +6201,15 @@ init_cumulative_args (CUMULATIVE_ARGS *cum,  /* Argument info to initialize */
>       FIXME: once typesytem is fixed, we won't need this code anymore.  */
>    if (i && i->local && i->can_change_signature)
>      fntype = TREE_TYPE (fndecl);
> +  cum->stdarg = fntype ? stdarg_p (fntype) : false;

No need for fntype check, stdarg_p already checks its argument, please
see tree.c.

>    cum->maybe_vaarg = (fntype
>                       ? (!prototype_p (fntype) || stdarg_p (fntype))
>                       : !libname);
>
> +  cum->bnd_regno = FIRST_BND_REG;
> +  cum->bnds_in_bt = 0;
> +  cum->force_bnd_pass = 0;
> +
>    if (!TARGET_64BIT)
>      {
>        /* If there are variable arguments, then we won't pass anything
> @@ -7136,13 +7144,17 @@ construct_container (enum machine_mode mode, enum machine_mode orig_mode,
>
>  /* Update the data in CUM to advance over an argument of mode MODE
>     and data type TYPE.  (TYPE is null for libcalls where that information
> -   may not be available.)  */
> +   may not be available.)
>
> -static void
> +   Return a number of integer regsiters advanced over.  */
> +
> +static int
>  function_arg_advance_32 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>                          const_tree type, HOST_WIDE_INT bytes,
>                          HOST_WIDE_INT words)
>  {
> +  int res = 0;
> +
>    switch (mode)
>      {
>      default:
> @@ -7160,7 +7172,8 @@ function_arg_advance_32 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>        cum->words += words;
>        cum->nregs -= words;
>        cum->regno += words;
> -
> +      if (cum->nregs >= 0)
> +       res = words;
>        if (cum->nregs <= 0)
>         {
>           cum->nregs = 0;
> @@ -7231,9 +7244,11 @@ function_arg_advance_32 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>         }
>        break;
>      }
> +
> +  return res;
>  }
>
> -static void
> +static int
>  function_arg_advance_64 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>                          const_tree type, HOST_WIDE_INT words, bool named)
>  {
> @@ -7242,7 +7257,7 @@ function_arg_advance_64 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>    /* Unnamed 512 and 256bit vector mode parameters are passed on stack.  */
>    if (!named && (VALID_AVX512F_REG_MODE (mode)
>                  || VALID_AVX256_REG_MODE (mode)))
> -    return;
> +    return 0;
>
>    if (!examine_argument (mode, type, 0, &int_nregs, &sse_nregs)
>        && sse_nregs <= cum->sse_nregs && int_nregs <= cum->nregs)
> @@ -7251,16 +7266,18 @@ function_arg_advance_64 (CUMULATIVE_ARGS *cum, enum machine_mode mode,
>        cum->sse_nregs -= sse_nregs;
>        cum->regno += int_nregs;
>        cum->sse_regno += sse_nregs;
> +      return int_nregs;
>      }
>    else
>      {
>        int align = ix86_function_arg_boundary (mode, type) / BITS_PER_WORD;
>        cum->words = (cum->words + align - 1) & ~(align - 1);
>        cum->words += words;
> +      return 0;
>      }
>  }
>
> -static void
> +static int
>  function_arg_advance_ms_64 (CUMULATIVE_ARGS *cum, HOST_WIDE_INT bytes,
>                             HOST_WIDE_INT words)
>  {
> @@ -7272,7 +7289,9 @@ function_arg_advance_ms_64 (CUMULATIVE_ARGS *cum, HOST_WIDE_INT bytes,
>      {
>        cum->nregs -= 1;
>        cum->regno += 1;
> +      return 1;
>      }
> +  return 0;
>  }
>
>  /* Update the data in CUM to advance over an argument of mode MODE and
> @@ -7285,6 +7304,7 @@ ix86_function_arg_advance (cumulative_args_t cum_v, enum machine_mode mode,
>  {
>    CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
>    HOST_WIDE_INT bytes, words;
> +  int nregs;
>
>    if (mode == BLKmode)
>      bytes = int_size_in_bytes (type);
> @@ -7295,12 +7315,51 @@ ix86_function_arg_advance (cumulative_args_t cum_v, enum machine_mode mode,
>    if (type)
>      mode = type_natural_mode (type, NULL, false);
>
> +  if ((type && POINTER_BOUNDS_TYPE_P (type))
> +      || POINTER_BOUNDS_MODE_P (mode))
> +    {
> +      /* If we pass bounds in BT then just update remained bounds count.  */
> +      if (cum->bnds_in_bt)
> +       {
> +         cum->bnds_in_bt--;
> +         return;
> +       }
> +
> +      /* Update remained number of bounds to force.  */
> +      if (cum->force_bnd_pass)
> +       cum->force_bnd_pass--;
> +
> +      cum->bnd_regno++;
> +
> +      return;
> +    }
> +
> +  /* The first arg not going to Bounds Tables resets this counter.  */
> +  cum->bnds_in_bt = 0;
> +  /* For unnamed args we always pass bounds to avoid bounds mess when
> +     passed and received types do not match.  If bounds do not follow
> +     unnamed arg, still pretend required number of bounds were passed.  */
> +  if (cum->force_bnd_pass)
> +    {
> +      cum->bnd_regno += cum->force_bnd_pass;
> +      cum->force_bnd_pass = 0;
> +    }
> +
>    if (TARGET_64BIT && (cum ? cum->call_abi : ix86_abi) == MS_ABI)
> -    function_arg_advance_ms_64 (cum, bytes, words);
> +    nregs = function_arg_advance_ms_64 (cum, bytes, words);
>    else if (TARGET_64BIT)
> -    function_arg_advance_64 (cum, mode, type, words, named);
> +    nregs = function_arg_advance_64 (cum, mode, type, words, named);
>    else
> -    function_arg_advance_32 (cum, mode, type, bytes, words);
> +    nregs = function_arg_advance_32 (cum, mode, type, bytes, words);
> +
> +  /* For stdarg we expect bounds to be passed for each value passed
> +     in register.  */
> +  if (cum->stdarg)
> +    cum->force_bnd_pass = nregs;
> +  /* For pointers passed in memory we expect bounds passed in Bounds
> +     Table.  */
> +  if (!nregs)
> +    cum->bnds_in_bt = chkp_type_bounds_count (type);
>  }
>
>  /* Define where to put the arguments to a function.
> @@ -7541,17 +7600,31 @@ ix86_function_arg (cumulative_args_t cum_v, enum machine_mode omode,
>      bytes = GET_MODE_SIZE (mode);
>    words = (bytes + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
>
> -  /* To simplify the code below, represent vector types with a vector mode
> -     even if MMX/SSE are not active.  */
> -  if (type && TREE_CODE (type) == VECTOR_TYPE)
> -    mode = type_natural_mode (type, cum, false);
>
> -  if (TARGET_64BIT && (cum ? cum->call_abi : ix86_abi) == MS_ABI)
> -    arg = function_arg_ms_64 (cum, mode, omode, named, bytes);
> -  else if (TARGET_64BIT)
> -    arg = function_arg_64 (cum, mode, omode, type, named);
> +  if ((type && POINTER_BOUNDS_TYPE_P (type))
> +      || POINTER_BOUNDS_MODE_P (mode))
> +    {
> +      if (cum->bnds_in_bt)
> +       arg = NULL;
> +      else if (cum->bnd_regno <= LAST_BND_REG)
> +       arg = gen_rtx_REG (BNDmode, cum->bnd_regno);
> +      else
> +       arg = GEN_INT (cum->bnd_regno - LAST_BND_REG - 1);

Please put early "return arg;" here, sou you don't have to reindend
existing code. Looking at surrounding code, I don't think cum is
always non-null. Also, put the whole block at the beginning of the
function.

> +    }
>    else
> -    arg = function_arg_32 (cum, mode, omode, type, bytes, words);
> +    {
> +      /* To simplify the code below, represent vector types with a vector mode
> +        even if MMX/SSE are not active.  */
> +      if (type && TREE_CODE (type) == VECTOR_TYPE)
> +       mode = type_natural_mode (type, cum, false);
> +
> +      if (TARGET_64BIT && (cum ? cum->call_abi : ix86_abi) == MS_ABI)
> +       arg = function_arg_ms_64 (cum, mode, omode, named, bytes);
> +      else if (TARGET_64BIT)
> +       arg = function_arg_64 (cum, mode, omode, type, named);
> +      else
> +       arg = function_arg_32 (cum, mode, omode, type, bytes, words);
> +    }
>
>    return arg;
>  }
> @@ -7808,6 +7881,9 @@ ix86_function_value_regno_p (const unsigned int regno)
>      case SI_REG:
>        return TARGET_64BIT && ix86_abi != MS_ABI;
>
> +    case FIRST_BND_REG:
> +      return chkp_function_instrumented_p (current_function_decl);
> +
>        /* Complex values are returned in %st(0)/%st(1) pair.  */
>      case ST0_REG:
>      case ST1_REG:
> @@ -7984,7 +8060,10 @@ ix86_function_value_1 (const_tree valtype, const_tree fntype_or_decl,
>      fn = fntype_or_decl;
>    fntype = fn ? TREE_TYPE (fn) : fntype_or_decl;
>
> -  if (TARGET_64BIT && ix86_function_type_abi (fntype) == MS_ABI)
> +  if ((valtype && POINTER_BOUNDS_TYPE_P (valtype))
> +      || POINTER_BOUNDS_MODE_P (mode))
> +    return gen_rtx_REG (BNDmode, FIRST_BND_REG);
> +  else if (TARGET_64BIT && ix86_function_type_abi (fntype) == MS_ABI)
>      return function_value_ms_64 (orig_mode, mode, valtype);
>    else if (TARGET_64BIT)
>      return function_value_64 (orig_mode, mode, valtype);
> @@ -8081,6 +8160,9 @@ ix86_return_in_memory (const_tree type, const_tree fntype ATTRIBUTE_UNUSED)
>    const enum machine_mode mode = type_natural_mode (type, NULL, true);
>    HOST_WIDE_INT size;
>
> +  if (POINTER_BOUNDS_TYPE_P (type))
> +    return false;
> +
>    if (TARGET_64BIT)
>      {
>        if (ix86_function_type_abi (fntype) == MS_ABI)
> @@ -15404,7 +15486,7 @@ ix86_print_operand (FILE *file, rtx x, int code)
>           return;
>
>         case '!':
> -         if (ix86_bnd_prefixed_insn_p (NULL_RTX))
> +         if (ix86_bnd_prefixed_insn_p (current_output_insn))
>             fputs ("bnd ", file);
>           return;
>
> @@ -25000,10 +25082,32 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx callarg1,
>      }
>
>    call = gen_rtx_CALL (VOIDmode, fnaddr, callarg1);
> +
>    if (retval)
> -    call = gen_rtx_SET (VOIDmode, retval, call);
> +    {
> +      /* For instrumented code we may have GPR + BR in parallel but
> +        it will confuse DF and we need to put each reg
> +        under EXPR_LIST.  */
> +      if (chkp_function_instrumented_p (current_function_decl))
> +       chkp_put_regs_to_expr_list (retval);

I assume that this is OK from infrastructure POV. I'd like to ask
Jeff, if this approach is OK.

> +
> +      call = gen_rtx_SET (VOIDmode, retval, call);
> +    }
>    vec[vec_len++] = call;
>
> +  /* b0 and b1 registers hold bounds for returned value.  */
> +  if (retval)
> +    {
> +      rtx b0 = gen_rtx_REG (BND64mode, FIRST_BND_REG);
> +      rtx unspec0 = gen_rtx_UNSPEC (BND64mode,
> +                                   gen_rtvec (1, b0), UNSPEC_BNDRET);
> +      rtx b1 = gen_rtx_REG (BND64mode, FIRST_BND_REG + 1);
> +      rtx unspec1 = gen_rtx_UNSPEC (BND64mode,
> +                                   gen_rtvec (1, b1), UNSPEC_BNDRET);
> +      vec[vec_len++] = gen_rtx_SET (BND64mode, b0, unspec0);
> +      vec[vec_len++] = gen_rtx_SET (BND64mode, b1, unspec1);
> +    }
> +
>    if (pop)
>      {
>        pop = gen_rtx_PLUS (Pmode, stack_pointer_rtx, pop);
> @@ -46053,9 +46157,18 @@ ix86_expand_sse2_mulvxdi3 (rtx op0, rtx op1, rtx op2)
>     bnd by default for current function.  */
>
>  bool
> -ix86_bnd_prefixed_insn_p (rtx insn ATTRIBUTE_UNUSED)
> +ix86_bnd_prefixed_insn_p (rtx insn)
>  {
> -  return false;
> +  /* For call insns check special flag.  */
> +  if (insn && CALL_P (insn))
> +    {
> +      rtx call = get_call_rtx_from (insn);
> +      if (call)
> +       return CALL_EXPR_WITH_BOUNDS_P (call);
> +    }
> +
> +  /* All other insns are prefixed only if function is instrumented.  */
> +  return chkp_function_instrumented_p (current_function_decl);
>  }
>
>  /* Calculate integer abs() using only SSE2 instructions.  */
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index a38c5d1..8f77343 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -1655,6 +1655,10 @@ typedef struct ix86_args {
>    int float_in_sse;            /* Set to 1 or 2 for 32bit targets if
>                                    SFmode/DFmode arguments should be passed
>                                    in SSE registers.  Otherwise 0.  */
> +  int bnd_regno;                /* next available bnd register number */
> +  int bnds_in_bt;               /* number of bounds expected in BT.  */
> +  int force_bnd_pass;           /* number of bounds expected for stdarg arg.  */
> +  int stdarg;                   /* Set to 1 if function is stdarg.  */
>    enum calling_abi call_abi;   /* Set to SYSV_ABI for sysv abi. Otherwise
>                                    MS_ABI for ms abi.  */
>  } CUMULATIVE_ARGS;
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index db22b06..0f3d555 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -194,6 +194,7 @@
>    UNSPEC_BNDCU
>    UNSPEC_BNDCN
>    UNSPEC_MPX_FENCE
> +  UNSPEC_BNDRET
>  ])
>
>  (define_c_enum "unspecv" [
> @@ -11549,7 +11550,9 @@
>  (define_insn "*call_value"
>    [(set (match_operand 0)
>         (call (mem:QI (match_operand:W 1 "call_insn_operand" "<c>BwBz"))
> -             (match_operand 2)))]
> +             (match_operand 2)))
> +   (set (reg:BND64 BND0_REG) (unspec [(reg:BND64 BND0_REG)] UNSPEC_BNDRET))
> +   (set (reg:BND64 BND1_REG) (unspec [(reg:BND64 BND1_REG)] UNSPEC_BNDRET))]

Does these patterns depend on BND0/BND1 values? If not, please use
(const_int 0) for their arguments.

OTOH, do you really need these sets here? Looking at the gcc internals
documentation (slightly unclear in this area), it should be enough to
list return values in EXPR_LIST. This question is somehow connected to
the new comment in ix86_expand_call, so perhaps Jeff can comment on
this approach.

>    "!SIBLING_CALL_P (insn)"
>    "* return ix86_output_call_insn (insn, operands[1]);"
>    [(set_attr "type" "callv")])
> @@ -11557,7 +11560,9 @@
>  (define_insn "*sibcall_value"
>    [(set (match_operand 0)
>         (call (mem:QI (match_operand:W 1 "sibcall_insn_operand" "UBsBz"))
> -             (match_operand 2)))]
> +             (match_operand 2)))
> +   (set (reg:BND64 BND0_REG) (unspec [(reg:BND64 BND0_REG)] UNSPEC_BNDRET))
> +   (set (reg:BND64 BND1_REG) (unspec [(reg:BND64 BND1_REG)] UNSPEC_BNDRET))]
>    "SIBLING_CALL_P (insn)"
>    "* return ix86_output_call_insn (insn, operands[1]);"
>    [(set_attr "type" "callv")])
> @@ -11604,6 +11609,8 @@
>      [(set (match_operand 0)
>           (call (mem:QI (match_operand:DI 1 "call_insn_operand" "rBwBz"))
>                 (match_operand 2)))
> +     (set (reg:BND64 BND0_REG) (unspec [(reg:BND64 BND0_REG)] UNSPEC_BNDRET))
> +     (set (reg:BND64 BND1_REG) (unspec [(reg:BND64 BND1_REG)] UNSPEC_BNDRET))
>       (unspec [(const_int 0)] UNSPEC_MS_TO_SYSV_CALL)])]
>   "TARGET_64BIT && !SIBLING_CALL_P (insn)"
>    "* return ix86_output_call_insn (insn, operands[1]);"
> @@ -11627,6 +11634,8 @@
>    [(set (match_operand 0)
>         (call (mem:QI (match_operand:SI 1 "call_insn_operand" "lmBz"))
>               (match_operand 2)))
> +   (set (reg:BND64 BND0_REG) (unspec [(reg:BND64 BND0_REG)] UNSPEC_BNDRET))
> +   (set (reg:BND64 BND1_REG) (unspec [(reg:BND64 BND1_REG)] UNSPEC_BNDRET))
>     (set (reg:SI SP_REG)
>         (plus:SI (reg:SI SP_REG)
>                  (match_operand:SI 3 "immediate_operand" "i")))]
> @@ -11638,6 +11647,8 @@
>    [(set (match_operand 0)
>         (call (mem:QI (match_operand:SI 1 "sibcall_insn_operand" "UBsBz"))
>               (match_operand 2)))
> +   (set (reg:BND64 BND0_REG) (unspec [(reg:BND64 BND0_REG)] UNSPEC_BNDRET))
> +   (set (reg:BND64 BND1_REG) (unspec [(reg:BND64 BND1_REG)] UNSPEC_BNDRET))
>     (set (reg:SI SP_REG)
>         (plus:SI (reg:SI SP_REG)
>                  (match_operand:SI 3 "immediate_operand" "i")))]
> diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
> index da01c9a..9991bb2 100644
> --- a/gcc/config/i386/predicates.md
> +++ b/gcc/config/i386/predicates.md
> @@ -631,14 +631,17 @@
>    (match_code "parallel")
>  {
>    unsigned creg_size = ARRAY_SIZE (x86_64_ms_sysv_extra_clobbered_registers);
> +  unsigned adop = GET_CODE (XVECEXP (op, 0, 0)) == SET
> +                  ? 4
> +                 : 2;
>    unsigned i;
>
> -  if ((unsigned) XVECLEN (op, 0) != creg_size + 2)
> +  if ((unsigned) XVECLEN (op, 0) != creg_size + adop)
>      return false;
>
>    for (i = 0; i < creg_size; i++)
>      {
> -      rtx elt = XVECEXP (op, 0, i+2);
> +      rtx elt = XVECEXP (op, 0, i+adop);
>        enum machine_mode mode;
>        unsigned regno;

Uros.



More information about the Gcc-patches mailing list