This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, i386, Pointer Bounds Checker 32/x] Pointer Bounds Checker hooks for i386 target


On Fri, Sep 19, 2014 at 2:53 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:

>> > This patch adds i386 target hooks for Pointer Bounds Checker.

> New version with fixes and better documentation for ix86_load_bounds and ix86_store_bounds is below.

> +/* Expand pass uses this hook to load bounds for function parameter
> +   PTR passed in SLOT in case its bounds are not passed in a register.
> +
> +   If SLOT is a memory, then bounds are loaded as for regular pointer
> +   loaded from memory.  PTR may be NULL in case SLOT is a memory.
> +   In such case value of PTR (if required) may be loaded from SLOT.
> +
> +   If SLOT is NULL or a register then SLOT_NO is an integer constant
> +   holding number of the target dependent special slot which should be
> +   used to obtain bounds.
> +
> +   Return loaded bounds.  */

OK, I hope I understand this target-handling of SLOT_NO. Can you
please clarify when SLOT is a register?

I propose to write this function in the following (hopefully equivalent) way:

--cut here--
{
  if (!slot)
    {
      gcc_assert (CONST_INT_P (slot_no));
      addr = plus_constant (Pmode, arg_pointer_rtx,
                -(INTVAL (slot_no) + 1) * GET_MODE_SIZE (Pmode));
      gcc_assert (ptr);
    }
  else if (REG_P (slot))
    {
      gcc_assert (CONST_INT_P (slot_no));
      addr = plus_constant (Pmode, arg_pointer_rtx,
                -(INTVAL (slot_no) + 1) * GET_MODE_SIZE (Pmode));
      ptr = slot;
    }
  else if (MEM_P (slot))
    {
      addr = XEXP (slot, 0);
      if (!register_operand (addr, Pmode))
    addr = copy_addr_to_reg (addr);

      if (!ptr)
    ptr = copy_addr_to_reg (slot);
    }
  else
    gcc_unreachable ();

  if (!index_register_operand (ptr, Pmode))
    ptr = copy_addr_to_reg (ptr);

  ...
}
--cut here--

Please add a comment in each of if/else, explaining what the code is
doing. This is non-trivial to understand.

> +
> +static rtx
> +ix86_load_bounds (rtx slot, rtx ptr, rtx slot_no)
> +{
> +  rtx addr = NULL;
> +  rtx reg;
> +
> +  if (!ptr)
> +    {
> +      gcc_assert (MEM_P (slot));
> +      ptr = copy_addr_to_reg (slot);
> +    }
> +
> +  if (!slot || REG_P (slot))
> +    {
> +      if (slot)
> +       ptr = slot;
> +
> +      gcc_assert (CONST_INT_P (slot_no));
> +
> +      /* Here we have the case when more than four pointers are
> +        passed in registers.  In this case we are out of bound
> +        registers and have to use bndldx to load bound.  RA,
> +        RA - 8, etc. are used for address translation in bndldx.  */
> +      addr = plus_constant (Pmode, arg_pointer_rtx,
> +                           -(INTVAL (slot_no) + 1) * GET_MODE_SIZE (Pmode));
> +    }
> +  else if (MEM_P (slot))
> +    {
> +      addr = XEXP (slot, 0);
> +      if (!register_operand (addr, Pmode))
> +       addr = copy_addr_to_reg (addr);
> +    }
> +  else
> +    gcc_unreachable ();
> +
> +  if (!register_operand (ptr, Pmode))
> +    ptr = copy_addr_to_reg (ptr);
> +
> +  reg = gen_reg_rtx (BNDmode);
> +  emit_insn (BNDmode == BND64mode
> +            ? gen_bnd64_ldx (reg, addr, ptr)
> +            : gen_bnd32_ldx (reg, addr, ptr));
> +
> +  return reg;
> +}
> +
> +/* Expand pass uses this hook to store BOUNDS for call argument PTR
> +   passed in SLOT in case BOUNDS are not passed in a register.
> +
> +   If SLOT is a memory, then BOUNDS are stored as for regular pointer
> +   stored in memory.  PTR may be NULL in case SLOT is a memory.
> +   In such case value of PTR (if required) may be loaded from SLOT.
> +
> +   If SLOT is NULL or a register then SLOT_NO is an integer constant
> +   holding number of the target dependent special slot which should be
> +   used to store BOUNDS.  */
> +
> +static void
> +ix86_store_bounds (rtx ptr, rtx slot, rtx bounds, rtx slot_no)

This function can be written in exactly the same way as the above
proposed code, up to the check for ptr register_operand.

> +{
> +  rtx addr;
> +
> +  if (ptr)
> +    {
> +      if (!register_operand (ptr, Pmode))
> +       ptr = copy_addr_to_reg (ptr);
> +    }
> +  else
> +    {
> +      gcc_assert (MEM_P (slot));
> +      ptr = copy_addr_to_reg (slot);
> +    }
> +
> +  if (!slot || REG_P (slot))
> +    {
> +      gcc_assert (CONST_INT_P (slot_no));
> +      addr = plus_constant (Pmode, stack_pointer_rtx,
> +                           -(INTVAL (slot_no) + 1) * GET_MODE_SIZE (Pmode));
> +    }
> +  else if (MEM_P (slot))
> +    addr = XEXP (slot, 0);
> +  else
> +    gcc_unreachable ();
> +
> +  if (!register_operand (addr, Pmode))
> +    addr = copy_addr_to_reg (addr);

The above check is needed since addr handling in the gen_bndX_stx
expander (patch 2/n) is different that addr handling in gen_bndX_ldx.
This handling should be unified in a follow-up patch.

> +  /* Avoid registers which connot be used as index.  */
> +  if (!index_register_operand (ptr, Pmode))
> +    {
> +      rtx temp = gen_reg_rtx (Pmode);
> +      emit_move_insn (temp, ptr);
> +      ptr = temp;
> +    }
> +
> +  gcc_assert (POINTER_BOUNDS_MODE_P (GET_MODE (bounds)));
> +  if (!register_operand (bounds, BNDmode))
> +    bounds = copy_to_mode_reg (BNDmode, bounds);
> +
> +  emit_insn (BNDmode == BND64mode
> +            ? gen_bnd64_stx (addr, ptr, bounds)
> +            : gen_bnd32_stx (addr, ptr, bounds));
> +}
> +
> +/* Load and return bounds returned by function in SLOT.  */
> +
> +static rtx
> +ix86_load_returned_bounds (rtx slot)
> +{
> +  rtx res;
> +
> +  gcc_assert (REG_P (slot));
> +  res = gen_reg_rtx (BNDmode);
> +  emit_move_insn (res, slot);
> +
> +  return res;
> +}
> +
> +/* Store BOUNDS returned by function into SLOT.  */
> +
> +static void
> +ix86_store_returned_bounds (rtx slot, rtx bounds)
> +{
> +  gcc_assert (REG_P (slot));
> +  emit_move_insn (slot, bounds);
> +}
> +
>  /* Returns a function decl for a vectorized version of the builtin function
>     with builtin function code FN and the result vector type TYPE, or NULL_TREE
>     if it is not available.  */
> @@ -47480,6 +47701,61 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
>                     atomic_feraiseexcept_call);
>  }
>
> +static enum machine_mode
> +ix86_mpx_bound_mode ()
> +{
> +  /* Do not support pointer checker if MPX
> +     is not enabled.  */
> +  if (!TARGET_MPX)
> +    {
> +      if (flag_check_pointer_bounds)
> +       warning (0, "Pointer Checker requires MPX support on this target."
> +                " Use -mmpx options to enable MPX.");
> +      return VOIDmode;
> +    }
> +
> +  return BNDmode;
> +}
> +
> +static tree
> +ix86_make_bounds_constant (HOST_WIDE_INT lb, HOST_WIDE_INT ub)
> +{
> +  tree low = lb ? build_minus_one_cst (pointer_sized_int_node)
> +    : build_zero_cst (pointer_sized_int_node);
> +  tree high = ub ? build_zero_cst (pointer_sized_int_node)
> +    : build_minus_one_cst (pointer_sized_int_node);
> +
> +  /* This function is supposed to be used to create zero and
> +     none bounds only.  */
> +  gcc_assert (lb == 0 || lb == -1);
> +  gcc_assert (ub == 0 || ub == -1);
> +
> +  return build_complex (NULL, low, high);
> +}
> +
> +static int
> +ix86_initialize_bounds (tree var, tree lb, tree ub, tree *stmts)
> +{
> +  tree size_ptr = build_pointer_type (size_type_node);
> +  tree lhs, modify, var_p;
> +
> +  ub = build1 (BIT_NOT_EXPR, size_type_node, ub);
> +  var_p = build1 (CONVERT_EXPR, size_ptr,
> +                 build_fold_addr_expr (var));
> +
> +  lhs = build1 (INDIRECT_REF, size_type_node, var_p);
> +  modify = build2 (MODIFY_EXPR, TREE_TYPE (lhs), lhs, lb);
> +  append_to_statement_list (modify, stmts);
> +
> +  lhs = build1 (INDIRECT_REF, size_type_node,
> +               build2 (POINTER_PLUS_EXPR, size_ptr, var_p,
> +                       TYPE_SIZE_UNIT (size_type_node)));
> +  modify = build2 (MODIFY_EXPR, TREE_TYPE (lhs), lhs, ub);
> +  append_to_statement_list (modify, stmts);
> +
> +  return 2;
> +}

The above two functions are outside of my expertise. Although they
look trivial, I'd be more comfortable if a tree expert reviews them.

> +
>  /* Initialize the GCC target structure.  */
>  #undef TARGET_RETURN_IN_MEMORY
>  #define TARGET_RETURN_IN_MEMORY ix86_return_in_memory
> @@ -47897,6 +48173,33 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
>  #undef TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
>  #define TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS true
>
> +#undef TARGET_LOAD_BOUNDS_FOR_ARG
> +#define TARGET_LOAD_BOUNDS_FOR_ARG ix86_load_bounds
> +
> +#undef TARGET_STORE_BOUNDS_FOR_ARG
> +#define TARGET_STORE_BOUNDS_FOR_ARG ix86_store_bounds
> +
> +#undef TARGET_LOAD_RETURNED_BOUNDS
> +#define TARGET_LOAD_RETURNED_BOUNDS ix86_load_returned_bounds
> +
> +#undef TARGET_STORE_RETURNED_BOUNDS
> +#define TARGET_STORE_RETURNED_BOUNDS ix86_store_returned_bounds
> +
> +#undef TARGET_CHKP_BOUND_MODE
> +#define TARGET_CHKP_BOUND_MODE ix86_mpx_bound_mode
> +
> +#undef TARGET_BUILTIN_CHKP_FUNCTION
> +#define TARGET_BUILTIN_CHKP_FUNCTION ix86_builtin_mpx_function
> +
> +#undef TARGET_CHKP_FUNCTION_VALUE_BOUNDS
> +#define TARGET_CHKP_FUNCTION_VALUE_BOUNDS ix86_function_value_bounds
> +
> +#undef TARGET_CHKP_MAKE_BOUNDS_CONSTANT
> +#define TARGET_CHKP_MAKE_BOUNDS_CONSTANT ix86_make_bounds_constant
> +
> +#undef TARGET_CHKP_INITIALIZE_BOUNDS
> +#define TARGET_CHKP_INITIALIZE_BOUNDS ix86_initialize_bounds
> +
>  struct gcc_target targetm = TARGET_INITIALIZER;
>
>  #include "gt-i386.h"

Uros.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]