Hi, Here is the patch that roughly follows your idea. Some comments:
- There are more cases than array_ref overflow. We need to take care of component_ref and both underflows/overflows are possible - I could not make it work with "0" as a fake address, because then catching lower bounds violation is getting hard at O2 and above. E.g. consider this: 0x00000000004005f8 <+8>: bndmk 0x7(%rax),%bnd0 0x00000000004005fd <+13>: mov $0x400734,%edi => 0x0000000000400602 <+18>: bndcl 0xfffffffffffffffc,%bnd0 (gdb) p $bnd0 $1 = {lbound = 0x0, ubound = 0x7} : size 8 0x000000000040060b <+27>: callq 0x400500 <printf@plt> - bndcu is removed as not necessary and underflowed access is not caught. I used another fake value for lower bound address, which is 2^(bitness - 1) - hard-reg-3-[1,2]* tests fail with ICE right now because of PR80270. I will mark them as XFAIL if the patch is approved and the mentioned bug is not fixed diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-lbv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-lbv.c new file mode 100644 index 0000000..319e1ec --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-lbv.c @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v16 __attribute__((vector_size(16))); + +int foo(int i) { + register v16 u asm("xmm0"); + return u[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (-1)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-nov.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-nov.c new file mode 100644 index 0000000..3c6d39a --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-nov.c @@ -0,0 +1,18 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + +#include "mpx-check.h" + +typedef int v16 __attribute__((vector_size(16))); + +int foo (int i) { + register v16 u asm ("xmm0"); + return u[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (3)); + printf ("%d\n", foo (0)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-ubv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-ubv.c new file mode 100644 index 0000000..7fe76c4 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-2-ubv.c @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v16 __attribute__((vector_size(16))); + +int foo (int i) { + register v16 u asm ("xmm0"); + return u[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (5)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-lbv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-lbv.c new file mode 100644 index 0000000..7e4451f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-lbv.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f1 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f1.s1f[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f1 (-1)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-nov.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-nov.c new file mode 100644 index 0000000..73bd7fb --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-nov.c @@ -0,0 +1,30 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f1 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f1.s1f[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f1 (0)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-ubv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-ubv.c new file mode 100644 index 0000000..166b6b9 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-1-ubv.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f1 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f1.s1f[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f1 (3)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-lbv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-lbv.c new file mode 100644 index 0000000..7820c2f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-lbv.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f2 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f2[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f2 (-1)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-nov.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-nov.c new file mode 100644 index 0000000..0816e58 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-nov.c @@ -0,0 +1,30 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f2 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f2[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f2 (0)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-ubv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-ubv.c new file mode 100644 index 0000000..94261a7 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-2-ubv.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S1 +{ + v8 s1f; +}; + +struct S2 +{ + struct S1 s2f1; + v8 s2f2; +}; + +int foo_s2f2 (int i) +{ + register struct S2 b asm ("xmm0"); + return b.s2f2[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s2f2 (3)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-lbv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-lbv.c new file mode 100644 index 0000000..f273d58 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-lbv.c @@ -0,0 +1,27 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v16 __attribute__ ((vector_size (16))); + +struct S1 +{ + v16 s1f1; +}; + +int foo_s1f1 (int i) +{ + register struct S1 b asm ("xmm0"); + return b.s1f1[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s1f1 (-1)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-nov.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-nov.c new file mode 100644 index 0000000..aa8f7b9 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-nov.c @@ -0,0 +1,24 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#include "mpx-check.h" + +typedef int v16 __attribute__ ((vector_size (16))); + +struct S1 +{ + v16 s1f1; +}; + +int foo_s1f1 (int i) +{ + register struct S1 b asm ("xmm0"); + return b.s1f1[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s1f1 (0)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-ubv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-ubv.c new file mode 100644 index 0000000..3d0c9b2 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-3-ubv.c @@ -0,0 +1,27 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v16 __attribute__ ((vector_size (16))); + +struct S1 +{ + v16 s1f1; +}; + +int foo_s1f1 (int i) +{ + register struct S1 b asm ("xmm0"); + return b.s1f1[i]; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo_s1f1 (7)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-lbv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-lbv.c new file mode 100644 index 0000000..e81b942 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-lbv.c @@ -0,0 +1,31 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S2 +{ + v8 s2f2; + int* f3; +}; + +int foo (int i) +{ + register struct S2 b asm ("xmm0"); + int k = 5; + b.f3 = &k; + b.f3 = b.f3 + i; + return *b.f3; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (-1)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-nov.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-nov.c new file mode 100644 index 0000000..4b1f1ac --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-nov.c @@ -0,0 +1,28 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S2 +{ + v8 s2f2; + int* f3; +}; + +int foo (int i) +{ + register struct S2 b asm ("xmm0"); + int k = 5; + b.f3 = &k; + b.f3 = b.f3 + i; + return *b.f3; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (0)); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-ubv.c b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-ubv.c new file mode 100644 index 0000000..e95e68f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/hard-reg-4-ubv.c @@ -0,0 +1,31 @@ +/* { dg-do run } */ +/* { dg-shouldfail "bounds violation" } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + + +#define SHOULDFAIL + +#include "mpx-check.h" + +typedef int v8 __attribute__ ((vector_size (8))); + +struct S2 +{ + v8 s2f2; + int* f3; +}; + +int foo (int i) +{ + register struct S2 b asm ("xmm0"); + int k = 5; + b.f3 = &k; + b.f3 = b.f3 + i; + return *b.f3; +} + +int mpx_test (int argc, const char **argv) +{ + printf ("%d\n", foo (1)); + return 0; +} diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index b1ff218..15c0da6 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -679,6 +679,17 @@ chkp_erase_completed_bounds (void) chkp_completed_bounds_set = new hash_set<tree>; } +/* If we check bounds for a hard register variable, we cannot + use its address - it is illegal, so instead of that we use + this fake value. */ +static tree +chkp_get_hard_register_var_fake_address () +{ + tree base = fold_convert (ptr_type_node, integer_zero_node); + unsigned HOST_WIDE_INT offset = 1 << (TYPE_PRECISION (ptr_type_node) - 1); + return fold_build_pointer_plus_hwi (base, offset); +} + /* Mark BOUNDS associated with PTR as incomplete. */ static void chkp_register_incomplete_bounds (tree bounds, tree ptr) @@ -1040,10 +1051,24 @@ chkp_add_modification_to_stmt_list (tree lhs, stmts->avail--; } -/* Build and return ADDR_EXPR for specified object OBJ. */ +/* Build and return ADDR_EXPR for specified object OBJ. + There is a special case for which we cannot return + ADDR_EXPR - if the object is declared to be placed + on a fixed hard register - in this case we cannot + take its address, so we use the object itself. The + caller of this function must be aware of that and + use proper checks if necessary. */ static tree chkp_build_addr_expr (tree obj) { + /* We first check whether it is a "hard reg case". */ + tree outer = obj; + while (TREE_CODE (outer) == COMPONENT_REF) + outer = TREE_OPERAND (outer, 0); + if (VAR_P (outer) && DECL_HARD_REGISTER (outer)) + return obj; + + /* If not - return regular ADDR_EXPR. */ return TREE_CODE (obj) == TARGET_MEM_REF ? tree_mem_ref_addr (ptr_type_node, obj) : build_fold_addr_expr (obj); @@ -3170,6 +3195,11 @@ chkp_get_bounds_for_decl_addr (tree decl) gcc_assert (VAR_P (decl)); bounds = chkp_generate_extern_var_bounds (decl); } + else if (VAR_P (decl) && DECL_HARD_REGISTER (decl)) + { + tree lb = chkp_get_hard_register_var_fake_address (); + bounds = chkp_make_bounds(lb, DECL_SIZE_UNIT (decl), NULL, false); + } else { tree lb = chkp_build_addr_expr (decl); @@ -3351,6 +3381,8 @@ chkp_narrow_bounds_to_field (tree bounds, tree component, tree field_ptr = chkp_build_addr_expr (component); tree field_bounds; + if (!BOUNDED_P (field_ptr)) + field_ptr = chkp_get_hard_register_var_fake_address (); field_bounds = chkp_make_bounds (field_ptr, size, iter, false); return chkp_intersect_bounds (field_bounds, bounds, iter); @@ -3639,6 +3671,8 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter) addr = chkp_build_addr_expr (ptr_src); bounds = chkp_build_bndldx (addr, ptr, iter); } + else if (VAR_P (ptr) && DECL_HARD_REGISTER (ptr)) + bounds = chkp_make_addressed_object_bounds (ptr_src, iter); else bounds = chkp_get_nonpointer_load_bounds (); break; @@ -4031,7 +4065,19 @@ chkp_process_stmt (gimple_stmt_iterator *iter, tree node, addr_first, byte_position (field)); } - else + else if (VAR_P (ptr) && DECL_HARD_REGISTER (ptr)) + { + gcc_assert (TREE_CODE (node) == ARRAY_REF + || TREE_CODE (node) == COMPONENT_REF); + + tree base = chkp_get_hard_register_var_fake_address (); + tree indx = fold_convert_loc (loc, + size_type_node, + TREE_OPERAND (node, 1)); + tree offset = size_binop_loc (loc, MULT_EXPR, size, indx); + addr_first = fold_build_pointer_plus_loc (loc, base, offset); + } + else addr_first = chkp_build_addr_expr (node); } break; 2017-03-23 20:57 GMT+01:00 Ilya Enkovich <enkovich....@gmail.com>: > 2017-03-23 17:18 GMT+03:00 Alexander Ivchenko <aivch...@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