Re: [CHKP] Fix for PR79990

[Ilya Enkovich <enkovich.gnu@gmail.com>]

2017-03-23 17:18 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
> Hi,
>
> The patch below attempts to fix the PR. I checked that it did not
> break any of mpx.exp tests, but I did not run the full testing yet.
> Would like to know whether this approach is generally correct or not.
>
> The issue is that we have the hard reg vector variable:
>
> typedef int U __attribute__ ((vector_size (16)));
> register U u asm("xmm0");
>
> and chkp tries to instrument access to it:
>
> return u[i];
>
> by doing that:
>
> __bound_tmp.0_4 = __builtin_ia32_bndmk (&u, 16);
>
> However, you cannot take an address of a register variable (in fact if
> you do that, the compiler will give you "address of register variable
> ‘u’ requested" error), so expand, sensibly, gives an ICE on on &u
> here. I believe that if there is no pointers, pointer bounds checker
> shouldn't get involved into that business. What do you think?

Hi!

I think with this patch I can call foo with any index and thus access
some random stack slot. The first thing we should answer is 'do we
want to catch array index overflows in such cases'? If we want to (and
this looks reasonable thing to do because it prevents invalid memory
accesses) then this patch doesn't resolve the problem.

I'm not sure it can affect the patch, but please consider more complex
cases. E.g.:

typedef int v8 __attribute__ ((vector_size(8)));

struct U {
  v8 a;
  v8 b;
};

int
foo (int i)
{
  register struct U u asm ("xmm0");
  return u.b[i];
}

One way to catch overflow in such cases might be to use some fake
pointer value (e.g. 0) for such not addressible variable. This fake value
would be used as base for memory access and as lower bound. I don't
see other cases except array_ref overflow check where such value
might be used. So this fake value will not be passed somewhere and
will not be stored to Bounds Table.

Thanks,
Ilya

>
>
>
>
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 75caf83..e39ec9a 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -3383,6 +3383,7 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>    tree comp_to_narrow = NULL_TREE;
>    tree last_comp = NULL_TREE;
>    bool array_ref_found = false;
> +  bool is_register_var = false;
>    tree *nodes;
>    tree var;
>    int len;
> @@ -3440,6 +3441,9 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>                   || TREE_CODE (var) == STRING_CST
>                   || TREE_CODE (var) == SSA_NAME);
>
> +      if (VAR_P (var) && DECL_HARD_REGISTER (var))
> +       is_register_var = true;
> +
>        *ptr = chkp_build_addr_expr (var);
>      }
>
> @@ -3455,7 +3459,11 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>
>        if (TREE_CODE (var) == ARRAY_REF)
>         {
> -         *safe = false;
> +         // Mark it as unsafe, unless the array being accessed
> +         // has been explicitly placed on a register: in this
> +         // case we cannot take a pointer of this variable,
> +         // so we don't instrument the access.
> +         *safe = is_register_var;
>           array_ref_found = true;
>           if (flag_chkp_narrow_bounds
>               && !flag_chkp_narrow_to_innermost_arrray
> @@ -4001,6 +4009,19 @@ chkp_process_stmt (gimple_stmt_iterator *iter, tree node,
>         bool bitfield;
>         tree elt;
>
> +       {
> +         // We don't instrument accesses to arrays that
> +         // are explicitely assigned to hard registers.
> +         HOST_WIDE_INT bitsize, bitpos;
> +         tree base, offset;
> +         machine_mode mode;
> +         int unsignedp, reversep, volatilep = 0;
> +         base = get_inner_reference (node, &bitsize, &bitpos, &offset, &mode,
> +                                     &unsignedp, &reversep, &volatilep);
> +         if (VAR_P (base) && DECL_HARD_REGISTER (base))
> +           safe = true;
> +       }
> +
>         if (safe)
>           {
>             /* We are not going to generate any checks, so do not
>
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79990.c
> b/gcc/testsuite/gcc.target/i386/mpx/pr79990.c
> new file mode 100644
> index 0000000..a27734d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/pr79990.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
> +
> +typedef int U __attribute__ ((vector_size (16)));
> +
> +int
> +foo (int i)
> +{
> +#if __SSE2__
> +  register
> +#endif
> +    U u
> +#if __SSE2__
> +      asm ("xmm0")
> +#endif
> +      ;
> +  return u[i];
> +}
>
> regards,
> Alexander