On Fri, Sep 19, 2014 at 2:53 PM, Ilya Enkovich <enkovich....@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.