On Mon, 22 Sep 2014, Uros Bizjak wrote: > On Mon, Sep 22, 2014 at 5:30 PM, Ilya Enkovich <enkovich....@gmail.com> wrote: > > On 19 Sep 18:21, Uros Bizjak wrote: > >> 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? > > > > For functions with more than four pointers passed in registers we do not > > have enough bound registers to pass bounds. These hooks are called then > > with SLOT set to register used to pass pointer > > > >> > >> I propose to write this function in the following (hopefully equivalent) > >> way: > > > > Since addr computation is very similar for both loading and storing (the > > only difference is usage of either arg_pointer_rtx or stack_pointer_rtx) I > > decided additionally move it into a separate function. This should make > > functions simplier for understanding. > > LGTM, just add the explanation when NULL is returned. > > > Here is an updated patch version. > > > > Thanks, > > Ilya > > -- > > 2014-09-22 Ilya Enkovich <ilya.enkov...@intel.com> > > > > * config/i386/i386.c: Include tree-iterator.h. > > (ix86_function_value_bounds): New. > > (ix86_builtin_mpx_function): New. > > (ix86_get_arg_address_for_bt): New. > > (ix86_load_bounds): New. > > (ix86_store_bounds): New. > > (ix86_load_returned_bounds): New. > > (ix86_store_returned_bounds): New. > > (ix86_mpx_bound_mode): New. > > (ix86_make_bounds_constant): New. > > (ix86_initialize_bounds): > > (TARGET_LOAD_BOUNDS_FOR_ARG): New. > > (TARGET_STORE_BOUNDS_FOR_ARG): New. > > (TARGET_LOAD_RETURNED_BOUNDS): New. > > (TARGET_STORE_RETURNED_BOUNDS): New. > > (TARGET_CHKP_BOUND_MODE): New. > > (TARGET_BUILTIN_CHKP_FUNCTION): New. > > (TARGET_CHKP_FUNCTION_VALUE_BOUNDS): New. > > (TARGET_CHKP_MAKE_BOUNDS_CONSTANT): New. > > (TARGET_CHKP_INITIALIZE_BOUNDS): New. > > Assuming that tree part is OK (I have CC'd Richi for his opinion), the > patch is OK. > > Thanks, > Uros. > > > > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > index d493983..8a3f577 100644 > > --- a/gcc/config/i386/i386.c > > +++ b/gcc/config/i386/i386.c > > @@ -85,6 +85,7 @@ along with GCC; see the file COPYING3. If not see > > #include "tree-vectorizer.h" > > #include "shrink-wrap.h" > > #include "builtins.h" > > +#include "tree-iterator.h" > > #include "tree-chkp.h" > > #include "rtl-chkp.h" > > > > @@ -8001,6 +8002,39 @@ ix86_function_value (const_tree valtype, const_tree > > fntype_or_decl, bool) > > return ix86_function_value_1 (valtype, fntype_or_decl, orig_mode, mode); > > } > >
The function misses a comment. > > +static rtx > > +ix86_function_value_bounds (const_tree valtype, > > + const_tree fntype_or_decl ATTRIBUTE_UNUSED, > > + bool outgoing ATTRIBUTE_UNUSED) > > +{ > > + rtx res = NULL_RTX; > > + > > + if (BOUNDED_TYPE_P (valtype)) > > + res = gen_rtx_REG (BNDmode, FIRST_BND_REG); > > + else if (chkp_type_has_pointer (valtype)) > > + { > > + bitmap slots = chkp_find_bound_slots (valtype); > > + rtx bounds[2]; > > + bitmap_iterator bi; > > + unsigned i, bnd_no = 0; > > + > > + EXECUTE_IF_SET_IN_BITMAP (slots, 0, i, bi) > > + { > > + rtx reg = gen_rtx_REG (BNDmode, FIRST_BND_REG + bnd_no); > > + rtx offs = GEN_INT (i * POINTER_SIZE / BITS_PER_UNIT); > > + gcc_assert (bnd_no < 2); > > + bounds[bnd_no++] = gen_rtx_EXPR_LIST (VOIDmode, reg, offs); > > + } > > + > > + res = gen_rtx_PARALLEL (VOIDmode, gen_rtvec_v (bnd_no, bounds)); > > + BITMAP_FREE (slots); > > + } > > + else > > + res = NULL_RTX; > > + > > + return res; > > +} > > + > > /* Pointer function arguments and return values are promoted to > > word_mode. */ > > > > @@ -36754,6 +36788,193 @@ static tree ix86_get_builtin (enum ix86_builtins > > code) > > return NULL_TREE; > > } > > > > +/* Return function decl for target specific builtin > > + for given MPX builtin passed i FCODE. */ > > +static tree > > +ix86_builtin_mpx_function (unsigned fcode) > > +{ > > + switch (fcode) > > + { > > + case BUILT_IN_CHKP_BNDMK: > > + return ix86_builtins[IX86_BUILTIN_BNDMK]; > > + > > + case BUILT_IN_CHKP_BNDSTX: > > + return ix86_builtins[IX86_BUILTIN_BNDSTX]; > > + > > + case BUILT_IN_CHKP_BNDLDX: > > + return ix86_builtins[IX86_BUILTIN_BNDLDX]; > > + > > + case BUILT_IN_CHKP_BNDCL: > > + return ix86_builtins[IX86_BUILTIN_BNDCL]; > > + > > + case BUILT_IN_CHKP_BNDCU: > > + return ix86_builtins[IX86_BUILTIN_BNDCU]; > > + > > + case BUILT_IN_CHKP_BNDRET: > > + return ix86_builtins[IX86_BUILTIN_BNDRET]; > > + > > + case BUILT_IN_CHKP_INTERSECT: > > + return ix86_builtins[IX86_BUILTIN_BNDINT]; > > + > > + case BUILT_IN_CHKP_NARROW: > > + return ix86_builtins[IX86_BUILTIN_BNDNARROW]; > > + > > + case BUILT_IN_CHKP_SIZEOF: > > + return ix86_builtins[IX86_BUILTIN_SIZEOF]; > > + > > + case BUILT_IN_CHKP_EXTRACT_LOWER: > > + return ix86_builtins[IX86_BUILTIN_BNDLOWER]; > > + > > + case BUILT_IN_CHKP_EXTRACT_UPPER: > > + return ix86_builtins[IX86_BUILTIN_BNDUPPER]; > > + > > + default: > > + return NULL_TREE; > > + } > > + > > + gcc_unreachable (); > > +} > > + > > +/* Helper function for ix86_load_bounds and ix86_store_bounds. > > + > > + Return an address to be used to load/store bounds for pointer > > + passed in SLOT. > > + > > + SLOT_NO is an integer constant holding number of a target > > + dependent special slot to be used in case SLOT is not a memory. > > + > > + SPECIAL_BASE is a pointer to be used as a base of fake address > > + to access special slots in Bounds Table. SPECIAL_BASE[-1], > > + SPECIAL_BASE[-2] etc. will be used as fake pointer locations. */ > > + > > +static rtx > > +ix86_get_arg_address_for_bt (rtx slot, rtx slot_no, rtx special_base) > > +{ > > + rtx addr = NULL; > > + > > + /* NULL slot means we pass bounds for pointer not passed to the > > + function at all. Register slot means we pass pointer in a > > + register. In both these cases bounds are passed via Bounds > > + Table. Since we do not have actual pointer stored in memory, > > + we have to use fake addresses to access Bounds Table. We > > + start with (special_base - sizeof (void*)) and decrease this > > + address by pointer size to get addresses for other slots. */ > > + if (!slot || REG_P (slot)) > > + { > > + gcc_assert (CONST_INT_P (slot_no)); > > + addr = plus_constant (Pmode, special_base, > > + -(INTVAL (slot_no) + 1) * GET_MODE_SIZE > > (Pmode)); > > + } > > + /* If pointer is passed in a memory then its address is used to > > + access Bounds Table. */ > > + else if (MEM_P (slot)) > > + { > > + addr = XEXP (slot, 0); > > + if (!register_operand (addr, Pmode)) > > + addr = copy_addr_to_reg (addr); > > + } > > + else > > + gcc_unreachable (); > > + > > + return addr; > > +} > > + > > +/* 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. */ > > + > > +static rtx > > +ix86_load_bounds (rtx slot, rtx ptr, rtx slot_no) > > +{ > > + rtx reg = gen_reg_rtx (BNDmode); > > + rtx addr; > > + > > + /* Get address to be used to access Bounds Table. Special slots start > > + at the location of return address of the current function. */ > > + addr = ix86_get_arg_address_for_bt (slot, slot_no, arg_pointer_rtx); > > + > > + /* Load pointer value from a memory if we don't have it. */ > > + if (!ptr) > > + { > > + gcc_assert (MEM_P (slot)); > > + ptr = copy_addr_to_reg (slot); > > + } > > + > > + 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) > > +{ > > + rtx addr; > > + > > + /* Get address to be used to access Bounds Table. Special slots start > > + at the location of return address of a called function. */ > > + addr = ix86_get_arg_address_for_bt (slot, slot_no, stack_pointer_rtx); > > + > > + /* Load pointer value from a memory if we don't have it. */ > > + if (!ptr) > > + { > > + gcc_assert (MEM_P (slot)); > > + ptr = copy_addr_to_reg (slot); > > + } > > + > > + 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); > > } > > Function misses a comment. > > +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; > > +} > > + Likewise. > > +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); > > +} > > + Likewise. > > +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)); Please use fold_convert (size_ptr, build_fold_addr_expr (var)). Is 'var' always accessed via a size_t effective type? Watch out for TBAA issues if not. (if it is, why is 'var' not of type size_t or size_t[]?) > > + 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); This was used from inside the gimplifier? Thus this is why you build GENERIC here and not GIMPLE? Didn't spot any other "tree" stuff. Richard. > > + return 2; > > +} > > + > > /* 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" > > -- Richard Biener <rguent...@suse.de> SUSE / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer