[PATCH 4/5] Disable -fcheck-pointer-bounds with sanitizers.

[PATCH 4/5] Disable -fcheck-pointer-bounds with sanitizers.

[marxin]

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/65705
	PR target/69804
	* toplev.c (process_options): Disable -fcheck-pointer-bounds with
	sanitizers.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/65705
	PR target/69804
	* gcc.target/i386/pr71458.c: Update scanned pattern.
---
 gcc/testsuite/gcc.target/i386/pr71458.c |  2 +-
 gcc/toplev.c                            | 29 +++++++++++++----------------
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/gcc/testsuite/gcc.target/i386/pr71458.c b/gcc/testsuite/gcc.target/i386/pr71458.c
index 27e7764b5a0..2faf6bb9391 100644
--- a/gcc/testsuite/gcc.target/i386/pr71458.c
+++ b/gcc/testsuite/gcc.target/i386/pr71458.c
@@ -1,6 +1,6 @@
 /* { dg-do compile { target { ! x32 } } } */
 /* { dg-options "-fcheck-pointer-bounds -mmpx -fsanitize=bounds" } */
-/* { dg-error "-fcheck-pointer-bounds is not supported with -fsanitize=bounds" "" { target *-*-* } 0 } */
+/* { dg-error "-fcheck-pointer-bounds is not supported with Undefined Behavior Sanitizer" "" { target *-*-* } 0 } */
 
 enum {} a[0];
 void fn1(int);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index beb581aba55..b8f87b878da 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1274,22 +1274,19 @@ process_options (void)
 	  flag_check_pointer_bounds = 0;
 	}
 
-      if (flag_sanitize & SANITIZE_ADDRESS)
-	{
-	  error_at (UNKNOWN_LOCATION,
-		    "-fcheck-pointer-bounds is not supported with "
-		    "Address Sanitizer");
-	  flag_check_pointer_bounds = 0;
-	}
-
-      if (flag_sanitize & SANITIZE_BOUNDS)
-	{
-	  error_at (UNKNOWN_LOCATION,
-		    "-fcheck-pointer-bounds is not supported with "
-		    "-fsanitize=bounds");
-	  flag_check_pointer_bounds = 0;
-	}
-
+      const char *sanitizer_names[] = { "Address", "Undefined Behavior",
+	"Leak", "Thread" };
+      const int sanitizer_flags[] = { SANITIZE_ADDRESS, SANITIZE_UNDEFINED,
+	SANITIZE_LEAK, SANITIZE_THREAD };
+
+      for (unsigned i = 0; i < sizeof (sanitizer_flags) / sizeof (int); i++)
+	if (flag_sanitize & sanitizer_flags[i])
+	  {
+	    error_at (UNKNOWN_LOCATION,
+		      "-fcheck-pointer-bounds is not supported with "
+		      "%s Sanitizer", sanitizer_names[i]);
+	    flag_check_pointer_bounds = 0;
+	  }
     }
 
   /* One region RA really helps to decrease the code size.  */
-- 
2.11.1

[PATCH 1/5] Fix *_CST ICEs connected to MPX.

[marxin]

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/79763
	PR target/79769
	PR target/79770
	* tree-chkp.c (chkp_find_bounds_1): Handle REAL_CST,
	COMPLEX_CST and VECTOR_CST.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/79763
	PR target/79769
	PR target/79770
	* g++.dg/pr79769.C: New test.
	* gcc.target/i386/mpx/pr79763.c: New test.
	* gcc.target/i386/mpx/pr79770.c: New test.
---
 gcc/testsuite/g++.dg/pr79769.C              |  4 ++++
 gcc/testsuite/gcc.target/i386/mpx/pr79763.c |  6 ++++++
 gcc/testsuite/gcc.target/i386/mpx/pr79770.c | 20 ++++++++++++++++++++
 gcc/tree-chkp.c                             |  3 +++
 4 files changed, 33 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/pr79769.C
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79763.c
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79770.c

diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
new file mode 100644
index 00000000000..f9223db1b2d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79769.C
@@ -0,0 +1,4 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+void a (_Complex) { a (3); }
diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79763.c b/gcc/testsuite/gcc.target/i386/mpx/pr79763.c
new file mode 100644
index 00000000000..59c2dececc2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79763.c
@@ -0,0 +1,6 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+int q_sk_num(void *a);
+typedef int (*fptr)(double);
+void a() { ((fptr)q_sk_num)(0); } /* { dg-warning "function called through a non-compatible type" } */
diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79770.c b/gcc/testsuite/gcc.target/i386/mpx/pr79770.c
new file mode 100644
index 00000000000..ede9abbbb8a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79770.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target lp64 } } */
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms -Wno-psabi" } */
+
+typedef unsigned U __attribute__ ((vector_size (64)));
+typedef unsigned __int128 V __attribute__ ((vector_size (64)));
+
+static inline V
+bar (U u, U x, V v)
+{
+  v = (V)(U) { 0, ~0 };
+  v[x[0]] <<= u[-63];
+  return v;
+}
+
+V
+foo (U u)
+{
+  return bar (u, (U) {}, (V) {});
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 02ae2d2d2c7..3d497f51ed8 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3665,6 +3665,9 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
       break;
 
     case INTEGER_CST:
+    case REAL_CST:
+    case COMPLEX_CST:
+    case VECTOR_CST:
       if (integer_zerop (ptr_src))
 	bounds = chkp_get_none_bounds ();
       else
-- 
2.11.1

[PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[marxin]

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
	(chkp_find_bounds_1): Remove gcc_unreachable.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* g++.dg/pr79761.C: New test.
---
 gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
 gcc/tree-chkp.c                |  3 +--
 2 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79761.C

diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
new file mode 100644
index 00000000000..b1f92d2b036
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79761.C
@@ -0,0 +1,34 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+struct Foo
+{
+  Foo() : a(1), b(1), c('a') {}
+  int a;
+  int b;
+  char c;
+};
+
+static Foo copy_foo(Foo) __attribute__((noinline, noclone));
+
+static Foo copy_foo(Foo A)
+{
+  return A;
+}
+
+struct Bar : Foo
+{
+  Bar(Foo t) : Foo(copy_foo(t)) {}
+};
+
+Foo F;
+
+int main (void)
+{
+  Bar B (F);
+
+  if (B.a != 1 || B.b != 1 || B.c != 'a')
+    __builtin_abort ();
+
+  return 0;
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 3d497f51ed8..d5683b1b9cf 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
 static tree
 chkp_get_bound_for_parm (tree parm)
 {
-  tree decl = SSA_NAME_VAR (parm);
+  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
   tree bounds;
 
   gcc_assert (TREE_CODE (decl) == PARM_DECL);
@@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
       break;
 
     case PARM_DECL:
-      gcc_unreachable ();
       bounds = chkp_get_bound_for_parm (ptr_src);
       break;
 
-- 
2.11.1

[PATCH 3/5] Fix ICE in tree-chkp-opt.c (PR tree-optimization/79631).

[marxin]

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/79631
	* tree-chkp-opt.c (chkp_is_constant_addr): Call
	tree_int_cst_sign_bit just for INTEGER constants.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/79631
	* gcc.target/i386/mpx/pr79631.c: New test.
---
 gcc/testsuite/gcc.target/i386/mpx/pr79631.c | 15 +++++++++++++++
 gcc/tree-chkp-opt.c                         |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79631.c

diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79631.c b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
new file mode 100644
index 00000000000..075d46b835f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
@@ -0,0 +1,15 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -O2" } */
+
+typedef struct { int _mp_size; } mpz_t[1];
+int a, b;
+void fn1()
+{
+  mpz_t c[1][b];
+  for (;;) {
+      int d = 0 >= 0 ? 0 == 0 ? c[0][0]->_mp_size ? -1 : 0 : 0 : 0,
+	  e = 0 >= 0 ? 0 == 0 ? c[1][1]->_mp_size ? -1 : 0 : 0 : 0;
+      if (d != e)
+	a++;
+  }
+}
diff --git a/gcc/tree-chkp-opt.c b/gcc/tree-chkp-opt.c
index ebe05459773..286f7853921 100644
--- a/gcc/tree-chkp-opt.c
+++ b/gcc/tree-chkp-opt.c
@@ -241,7 +241,8 @@ chkp_is_constant_addr (const address_t &addr, int *sign)
     return false;
   else if (integer_zerop (addr.pol[0].cst))
     *sign = 0;
-  else if  (tree_int_cst_sign_bit (addr.pol[0].cst))
+  else if (TREE_CODE (addr.pol[0].cst) == INTEGER_CST
+	   && tree_int_cst_sign_bit (addr.pol[0].cst))
     *sign = -1;
   else
     *sign = 1;
-- 
2.11.1

[PATCH 5/5] Support BIT_FIELD_REF in MPX (PR ipa/79764).

[marxin]

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79764
	* tree-chkp.c (chkp_narrow_bounds_for_field): Fix typo in
	comment.
	(chkp_narrow_size_and_offset): New function.
	(chkp_parse_array_and_component_ref): Support BIT_FIELD_REF.
	(void chkp_parse_bit_field_ref): New function.
	(chkp_make_addressed_object_bounds): Add case for BIT_FIELD_REF.
	(chkp_process_stmt): Use chkp_parse_bit_field_ref.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79764
	* g++.dg/pr79764.C: New test.
---
 gcc/testsuite/g++.dg/pr79764.C | 12 ++++++
 gcc/tree-chkp.c                | 90 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 85 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79764.C

diff --git a/gcc/testsuite/g++.dg/pr79764.C b/gcc/testsuite/g++.dg/pr79764.C
new file mode 100644
index 00000000000..47fb88da19b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79764.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
+
+typedef float __m256 __attribute__ (( __vector_size__(32), __may_alias__ ));
+struct A {
+  __m256 ymm;
+  const float &f() const;
+};
+
+const float &A::f() const {
+  return ymm[1];
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index d5683b1b9cf..14ebff294f9 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -325,6 +325,8 @@ static void chkp_parse_array_and_component_ref (tree node, tree *ptr,
 						tree *bounds,
 						gimple_stmt_iterator *iter,
 						bool innermost_bounds);
+static void chkp_parse_bit_field_ref (tree node, location_t loc,
+				      tree *offset, tree *size);
 
 #define chkp_bndldx_fndecl \
   (targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDLDX))
@@ -3266,7 +3268,7 @@ chkp_intersect_bounds (tree bounds1, tree bounds2, gimple_stmt_iterator *iter)
 }
 
 /* Return 1 if we are allowed to narrow bounds for addressed FIELD
-   and 0 othersize.  */
+   and 0 otherwise.  */
 static bool
 chkp_may_narrow_to_field (tree field)
 {
@@ -3294,7 +3296,7 @@ chkp_narrow_bounds_for_field (tree field)
   if (!chkp_may_narrow_to_field (field))
     return false;
 
-  /* Accesse to compiler generated fields should not cause
+  /* Access to compiler generated fields should not cause
      bounds narrowing.  */
   if (DECL_ARTIFICIAL (field))
     return false;
@@ -3308,9 +3310,36 @@ chkp_narrow_bounds_for_field (tree field)
 	      || bit_offs));
 }
 
+/* Perform narrowing for BOUNDS of an INNER reference.  Shift boundary
+   by OFFSET bytes and limit to SIZE bytes.  Newly created statements are
+   added to ITER.  */
+
+static tree
+chkp_narrow_size_and_offset (tree bounds, tree inner, tree offset,
+			     tree size, gimple_stmt_iterator *iter)
+{
+  tree addr = chkp_build_addr_expr (unshare_expr (inner));
+  tree t = TREE_TYPE (addr);
+
+  gimple *stmt = gimple_build_assign (NULL_TREE, addr);
+  addr = make_temp_ssa_name (t, stmt, CHKP_BOUND_TMP_NAME);
+  gimple_assign_set_lhs (stmt, addr);
+  gsi_insert_seq_before (iter, stmt, GSI_SAME_STMT);
+
+  stmt = gimple_build_assign (NULL_TREE, POINTER_PLUS_EXPR, addr, offset);
+  tree shifted = make_temp_ssa_name (t, stmt, CHKP_BOUND_TMP_NAME);
+  gimple_assign_set_lhs (stmt, shifted);
+  gsi_insert_seq_before (iter, stmt, GSI_SAME_STMT);
+
+  tree bounds2 = chkp_make_bounds (shifted, size, iter, false);
+
+  return chkp_intersect_bounds (bounds, bounds2, iter);
+}
+
 /* Perform narrowing for BOUNDS using bounds computed for field
    access COMPONENT.  ITER meaning is the same as for
    chkp_intersect_bounds.  */
+
 static tree
 chkp_narrow_bounds_to_field (tree bounds, tree component,
 			    gimple_stmt_iterator *iter)
@@ -3363,7 +3392,8 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
   len = 1;
   while (TREE_CODE (var) == COMPONENT_REF
 	 || TREE_CODE (var) == ARRAY_REF
-	 || TREE_CODE (var) == VIEW_CONVERT_EXPR)
+	 || TREE_CODE (var) == VIEW_CONVERT_EXPR
+	 || TREE_CODE (var) == BIT_FIELD_REF)
     {
       var = TREE_OPERAND (var, 0);
       len++;
@@ -3382,9 +3412,10 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
   if (bounds)
     *bounds = NULL;
   *safe = true;
-  *bitfield = (TREE_CODE (node) == COMPONENT_REF
-	       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (node, 1)));
-  /* To get bitfield address we will need outer elemnt.  */
+  *bitfield = ((TREE_CODE (node) == COMPONENT_REF
+	       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (node, 1)))
+	       || TREE_CODE (node) == BIT_FIELD_REF);
+  /* To get bitfield address we will need outer element.  */
   if (*bitfield)
     *elt = nodes[len - 2];
   else
@@ -3453,6 +3484,17 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
 	      comp_to_narrow = NULL;
 	    }
 	}
+      else if (TREE_CODE (var) == BIT_FIELD_REF)
+	{
+	  if (flag_chkp_narrow_bounds && bounds)
+	    {
+	      tree offset, size;
+	      chkp_parse_bit_field_ref (var, UNKNOWN_LOCATION, &offset, &size);
+	      *bounds
+		= chkp_narrow_size_and_offset (*bounds, TREE_OPERAND (var, 0),
+					       offset, size, iter);
+	    }
+	}
       else if (TREE_CODE (var) == VIEW_CONVERT_EXPR)
 	/* Nothing to do for it.  */
 	;
@@ -3467,6 +3509,27 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
     *bounds = chkp_find_bounds (*ptr, iter);
 }
 
+/* Parse BIT_FIELD_REF to a NODE for a given location LOC.  Return OFFSET
+   and SIZE in bytes.  */
+
+static
+void chkp_parse_bit_field_ref (tree node, location_t loc, tree *offset,
+			       tree *size)
+{
+  tree bpu = fold_convert (size_type_node, bitsize_int (BITS_PER_UNIT));
+  tree offs = fold_convert (size_type_node, TREE_OPERAND (node, 2));
+  tree rem = size_binop_loc (loc, TRUNC_MOD_EXPR, offs, bpu);
+  offs = size_binop_loc (loc, TRUNC_DIV_EXPR, offs, bpu);
+
+  tree s = fold_convert (size_type_node, TREE_OPERAND (node, 1));
+  s = size_binop_loc (loc, PLUS_EXPR, s, rem);
+  s = size_binop_loc (loc, CEIL_DIV_EXPR, s, bpu);
+  s = fold_convert (size_type_node, s);
+
+  *offset = offs;
+  *size = s;
+}
+
 /* Compute and return bounds for address of OBJ.  */
 static tree
 chkp_make_addressed_object_bounds (tree obj, gimple_stmt_iterator *iter)
@@ -3490,6 +3553,7 @@ chkp_make_addressed_object_bounds (tree obj, gimple_stmt_iterator *iter)
 
     case ARRAY_REF:
     case COMPONENT_REF:
+    case BIT_FIELD_REF:
       {
 	tree elt;
 	tree ptr;
@@ -3993,23 +4057,15 @@ chkp_process_stmt (gimple_stmt_iterator *iter, tree node,
 
     case BIT_FIELD_REF:
       {
-	tree offs, rem, bpu;
+	tree offset, size;
 
 	gcc_assert (!access_offs);
 	gcc_assert (!access_size);
 
-	bpu = fold_convert (size_type_node, bitsize_int (BITS_PER_UNIT));
-	offs = fold_convert (size_type_node, TREE_OPERAND (node, 2));
-	rem = size_binop_loc (loc, TRUNC_MOD_EXPR, offs, bpu);
-	offs = size_binop_loc (loc, TRUNC_DIV_EXPR, offs, bpu);
-
-	size = fold_convert (size_type_node, TREE_OPERAND (node, 1));
-        size = size_binop_loc (loc, PLUS_EXPR, size, rem);
-        size = size_binop_loc (loc, CEIL_DIV_EXPR, size, bpu);
-        size = fold_convert (size_type_node, size);
+	chkp_parse_bit_field_ref (node, loc, &offset, &size);
 
 	chkp_process_stmt (iter, TREE_OPERAND (node, 0), loc,
-			 dirflag, offs, size, safe);
+			   dirflag, offset, size, safe);
 	return;
       }
       break;
-- 
2.11.1

[PATCH 0/5] Fix various MPX issues

[marxin]

Hello.

I'm sending a small series that fixes MPX issue that are quite easily fixable.
The series can bootstrap on ppc64le and survives regression tests.

Thanks for review,
Martin

marxin (5):
  Fix *_CST ICEs connected to MPX.
  Get bounds for a PARM_DECL (PR ipa/79761).
  Fix ICE in tree-chkp-opt.c (PR tree-optimization/79631).
  Disable -fcheck-pointer-bounds with sanitizers.
  Support BIT_FIELD_REF in MPX (PR ipa/79764).

 gcc/testsuite/g++.dg/pr79761.C              | 34 ++++++++++
 gcc/testsuite/g++.dg/pr79764.C              | 12 ++++
 gcc/testsuite/g++.dg/pr79769.C              |  4 ++
 gcc/testsuite/gcc.target/i386/mpx/pr79631.c | 15 +++++
 gcc/testsuite/gcc.target/i386/mpx/pr79763.c |  6 ++
 gcc/testsuite/gcc.target/i386/mpx/pr79770.c | 20 ++++++
 gcc/testsuite/gcc.target/i386/pr71458.c     |  2 +-
 gcc/toplev.c                                | 29 ++++-----
 gcc/tree-chkp-opt.c                         |  3 +-
 gcc/tree-chkp.c                             | 96 +++++++++++++++++++++++------
 10 files changed, 184 insertions(+), 37 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79761.C
 create mode 100644 gcc/testsuite/g++.dg/pr79764.C
 create mode 100644 gcc/testsuite/g++.dg/pr79769.C
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79631.c
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79763.c
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79770.c

-- 
2.11.1

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Rainer Orth]

marxin <mliska@suse.cz> writes:

> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
> new file mode 100644
> index 00000000000..b1f92d2b036
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr79761.C
> @@ -0,0 +1,34 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */

This isn't right: the test must be restricted to x86 targets like
g++.dg/pr71633.C.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Rainer Orth]

marxin <mliska@suse.cz> writes:

> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
> new file mode 100644
> index 00000000000..f9223db1b2d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr79769.C
> @@ -0,0 +1,4 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */

... and again: make this x86-only.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 5/5] Support BIT_FIELD_REF in MPX (PR ipa/79764).

[Rainer Orth]

marxin <mliska@suse.cz> writes:

> diff --git a/gcc/testsuite/g++.dg/pr79764.C b/gcc/testsuite/g++.dg/pr79764.C
> new file mode 100644
> index 00000000000..47fb88da19b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr79764.C
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */

Same here.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On 03/07/2017 11:16 AM, Rainer Orth wrote:
> marxin <mliska@suse.cz> writes:
> 
>> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
>> new file mode 100644
>> index 00000000000..b1f92d2b036
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr79761.C
>> @@ -0,0 +1,34 @@
>> +/* { dg-do compile { target { ! x32 } } } */
>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
> 
> This isn't right: the test must be restricted to x86 targets like
> g++.dg/pr71633.C.
> 
> 	Rainer
> 

Thanks. I'm sending v2 of the patch.

Martin

[-- Attachment #2: 0002-Get-bounds-for-a-PARM_DECL-PR-ipa-79761-v2.patch --]
[-- Type: text/x-patch, Size: 2112 bytes --]

From e42fa451780336b06bd8fb63ab352a9cb72323ee Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 2 Mar 2017 18:06:39 +0100
Subject: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
	(chkp_find_bounds_1): Remove gcc_unreachable.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* g++.dg/pr79761.C: New test.
---
 gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
 gcc/tree-chkp.c                |  3 +--
 2 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79761.C

diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
new file mode 100644
index 00000000000..a97325a1fc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79761.C
@@ -0,0 +1,34 @@
+/* { dg-do compile { target { { i?86-*-* x86_64-*-* } && { ! x32 } } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+struct Foo
+{
+  Foo() : a(1), b(1), c('a') {}
+  int a;
+  int b;
+  char c;
+};
+
+static Foo copy_foo(Foo) __attribute__((noinline, noclone));
+
+static Foo copy_foo(Foo A)
+{
+  return A;
+}
+
+struct Bar : Foo
+{
+  Bar(Foo t) : Foo(copy_foo(t)) {}
+};
+
+Foo F;
+
+int main (void)
+{
+  Bar B (F);
+
+  if (B.a != 1 || B.b != 1 || B.c != 'a')
+    __builtin_abort ();
+
+  return 0;
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 3d497f51ed8..d5683b1b9cf 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
 static tree
 chkp_get_bound_for_parm (tree parm)
 {
-  tree decl = SSA_NAME_VAR (parm);
+  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
   tree bounds;
 
   gcc_assert (TREE_CODE (decl) == PARM_DECL);
@@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
       break;
 
     case PARM_DECL:
-      gcc_unreachable ();
       bounds = chkp_get_bound_for_parm (ptr_src);
       break;
 
-- 
2.11.1

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On 03/07/2017 11:17 AM, Rainer Orth wrote:
> marxin <mliska@suse.cz> writes:
> 
>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>> new file mode 100644
>> index 00000000000..f9223db1b2d
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>> @@ -0,0 +1,4 @@
>> +/* { dg-do compile { target { ! x32 } } } */
>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
> 
> ... and again: make this x86-only.
> 
> 	Rainer
> 

Thanks. I'm sending v2 of the patch.

Martin

[-- Attachment #2: 0001-Fix-_CST-ICEs-connected-to-MPX-v2.patch --]
[-- Type: text/x-patch, Size: 3013 bytes --]

From 7abb81c9836562344f49544a98a8afd28a6203e4 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 2 Mar 2017 17:59:06 +0100
Subject: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/79763
	PR target/79769
	PR target/79770
	* tree-chkp.c (chkp_find_bounds_1): Handle REAL_CST,
	COMPLEX_CST and VECTOR_CST.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/79763
	PR target/79769
	PR target/79770
	* g++.dg/pr79769.C: New test.
	* gcc.target/i386/mpx/pr79763.c: New test.
	* gcc.target/i386/mpx/pr79770.c: New test.
---
 gcc/testsuite/g++.dg/pr79769.C              |  4 ++++
 gcc/testsuite/gcc.target/i386/mpx/pr79763.c |  6 ++++++
 gcc/testsuite/gcc.target/i386/mpx/pr79770.c | 19 +++++++++++++++++++
 gcc/tree-chkp.c                             |  3 +++
 4 files changed, 32 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/pr79769.C
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79763.c
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79770.c

diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
new file mode 100644
index 00000000000..c3186877f60
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79769.C
@@ -0,0 +1,4 @@
+/* { dg-do compile { target { { i?86-*-* x86_64-*-* } && { ! x32 } } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+void a (_Complex) { a (3); }
diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79763.c b/gcc/testsuite/gcc.target/i386/mpx/pr79763.c
new file mode 100644
index 00000000000..59c2dececc2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79763.c
@@ -0,0 +1,6 @@
+/* { dg-do compile { target { ! x32 } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+int q_sk_num(void *a);
+typedef int (*fptr)(double);
+void a() { ((fptr)q_sk_num)(0); } /* { dg-warning "function called through a non-compatible type" } */
diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79770.c b/gcc/testsuite/gcc.target/i386/mpx/pr79770.c
new file mode 100644
index 00000000000..0890fcc7bf1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79770.c
@@ -0,0 +1,19 @@
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms -Wno-psabi" } */
+
+typedef unsigned U __attribute__ ((vector_size (64)));
+typedef unsigned __int128 V __attribute__ ((vector_size (64)));
+
+static inline V
+bar (U u, U x, V v)
+{
+  v = (V)(U) { 0, ~0 };
+  v[x[0]] <<= u[-63];
+  return v;
+}
+
+V
+foo (U u)
+{
+  return bar (u, (U) {}, (V) {});
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 02ae2d2d2c7..3d497f51ed8 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3665,6 +3665,9 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
       break;
 
     case INTEGER_CST:
+    case REAL_CST:
+    case COMPLEX_CST:
+    case VECTOR_CST:
       if (integer_zerop (ptr_src))
 	bounds = chkp_get_none_bounds ();
       else
-- 
2.11.1

Re: [PATCH 5/5] Support BIT_FIELD_REF in MPX (PR ipa/79764).

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On 03/07/2017 11:18 AM, Rainer Orth wrote:
> marxin <mliska@suse.cz> writes:
> 
>> diff --git a/gcc/testsuite/g++.dg/pr79764.C b/gcc/testsuite/g++.dg/pr79764.C
>> new file mode 100644
>> index 00000000000..47fb88da19b
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr79764.C
>> @@ -0,0 +1,12 @@
>> +/* { dg-do compile { target { ! x32 } } } */
>> +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
> 
> Same here.
> 
> 	Rainer
> 

Thanks. I'm sending v2 of the patch.

Martin

[-- Attachment #2: 0005-Support-BIT_FIELD_REF-in-MPX-PR-ipa-79764-v2.patch --]
[-- Type: text/x-patch, Size: 7548 bytes --]

From 7c54acbb0a2d36a3d1676533937d78b3e0a40874 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 2 Mar 2017 17:52:03 +0100
Subject: [PATCH 5/5] Support BIT_FIELD_REF in MPX (PR ipa/79764).

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79764
	* tree-chkp.c (chkp_narrow_bounds_for_field): Fix typo in
	comment.
	(chkp_narrow_size_and_offset): New function.
	(chkp_parse_array_and_component_ref): Support BIT_FIELD_REF.
	(void chkp_parse_bit_field_ref): New function.
	(chkp_make_addressed_object_bounds): Add case for BIT_FIELD_REF.
	(chkp_process_stmt): Use chkp_parse_bit_field_ref.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79764
	* g++.dg/pr79764.C: New test.
---
 gcc/testsuite/g++.dg/pr79764.C | 12 ++++++
 gcc/tree-chkp.c                | 90 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 85 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79764.C

diff --git a/gcc/testsuite/g++.dg/pr79764.C b/gcc/testsuite/g++.dg/pr79764.C
new file mode 100644
index 00000000000..293aa337693
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79764.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target { { i?86-*-* x86_64-*-* } && { ! x32 } } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
+
+typedef float __m256 __attribute__ (( __vector_size__(32), __may_alias__ ));
+struct A {
+  __m256 ymm;
+  const float &f() const;
+};
+
+const float &A::f() const {
+  return ymm[1];
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index d5683b1b9cf..14ebff294f9 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -325,6 +325,8 @@ static void chkp_parse_array_and_component_ref (tree node, tree *ptr,
 						tree *bounds,
 						gimple_stmt_iterator *iter,
 						bool innermost_bounds);
+static void chkp_parse_bit_field_ref (tree node, location_t loc,
+				      tree *offset, tree *size);
 
 #define chkp_bndldx_fndecl \
   (targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDLDX))
@@ -3266,7 +3268,7 @@ chkp_intersect_bounds (tree bounds1, tree bounds2, gimple_stmt_iterator *iter)
 }
 
 /* Return 1 if we are allowed to narrow bounds for addressed FIELD
-   and 0 othersize.  */
+   and 0 otherwise.  */
 static bool
 chkp_may_narrow_to_field (tree field)
 {
@@ -3294,7 +3296,7 @@ chkp_narrow_bounds_for_field (tree field)
   if (!chkp_may_narrow_to_field (field))
     return false;
 
-  /* Accesse to compiler generated fields should not cause
+  /* Access to compiler generated fields should not cause
      bounds narrowing.  */
   if (DECL_ARTIFICIAL (field))
     return false;
@@ -3308,9 +3310,36 @@ chkp_narrow_bounds_for_field (tree field)
 	      || bit_offs));
 }
 
+/* Perform narrowing for BOUNDS of an INNER reference.  Shift boundary
+   by OFFSET bytes and limit to SIZE bytes.  Newly created statements are
+   added to ITER.  */
+
+static tree
+chkp_narrow_size_and_offset (tree bounds, tree inner, tree offset,
+			     tree size, gimple_stmt_iterator *iter)
+{
+  tree addr = chkp_build_addr_expr (unshare_expr (inner));
+  tree t = TREE_TYPE (addr);
+
+  gimple *stmt = gimple_build_assign (NULL_TREE, addr);
+  addr = make_temp_ssa_name (t, stmt, CHKP_BOUND_TMP_NAME);
+  gimple_assign_set_lhs (stmt, addr);
+  gsi_insert_seq_before (iter, stmt, GSI_SAME_STMT);
+
+  stmt = gimple_build_assign (NULL_TREE, POINTER_PLUS_EXPR, addr, offset);
+  tree shifted = make_temp_ssa_name (t, stmt, CHKP_BOUND_TMP_NAME);
+  gimple_assign_set_lhs (stmt, shifted);
+  gsi_insert_seq_before (iter, stmt, GSI_SAME_STMT);
+
+  tree bounds2 = chkp_make_bounds (shifted, size, iter, false);
+
+  return chkp_intersect_bounds (bounds, bounds2, iter);
+}
+
 /* Perform narrowing for BOUNDS using bounds computed for field
    access COMPONENT.  ITER meaning is the same as for
    chkp_intersect_bounds.  */
+
 static tree
 chkp_narrow_bounds_to_field (tree bounds, tree component,
 			    gimple_stmt_iterator *iter)
@@ -3363,7 +3392,8 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
   len = 1;
   while (TREE_CODE (var) == COMPONENT_REF
 	 || TREE_CODE (var) == ARRAY_REF
-	 || TREE_CODE (var) == VIEW_CONVERT_EXPR)
+	 || TREE_CODE (var) == VIEW_CONVERT_EXPR
+	 || TREE_CODE (var) == BIT_FIELD_REF)
     {
       var = TREE_OPERAND (var, 0);
       len++;
@@ -3382,9 +3412,10 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
   if (bounds)
     *bounds = NULL;
   *safe = true;
-  *bitfield = (TREE_CODE (node) == COMPONENT_REF
-	       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (node, 1)));
-  /* To get bitfield address we will need outer elemnt.  */
+  *bitfield = ((TREE_CODE (node) == COMPONENT_REF
+	       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (node, 1)))
+	       || TREE_CODE (node) == BIT_FIELD_REF);
+  /* To get bitfield address we will need outer element.  */
   if (*bitfield)
     *elt = nodes[len - 2];
   else
@@ -3453,6 +3484,17 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
 	      comp_to_narrow = NULL;
 	    }
 	}
+      else if (TREE_CODE (var) == BIT_FIELD_REF)
+	{
+	  if (flag_chkp_narrow_bounds && bounds)
+	    {
+	      tree offset, size;
+	      chkp_parse_bit_field_ref (var, UNKNOWN_LOCATION, &offset, &size);
+	      *bounds
+		= chkp_narrow_size_and_offset (*bounds, TREE_OPERAND (var, 0),
+					       offset, size, iter);
+	    }
+	}
       else if (TREE_CODE (var) == VIEW_CONVERT_EXPR)
 	/* Nothing to do for it.  */
 	;
@@ -3467,6 +3509,27 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
     *bounds = chkp_find_bounds (*ptr, iter);
 }
 
+/* Parse BIT_FIELD_REF to a NODE for a given location LOC.  Return OFFSET
+   and SIZE in bytes.  */
+
+static
+void chkp_parse_bit_field_ref (tree node, location_t loc, tree *offset,
+			       tree *size)
+{
+  tree bpu = fold_convert (size_type_node, bitsize_int (BITS_PER_UNIT));
+  tree offs = fold_convert (size_type_node, TREE_OPERAND (node, 2));
+  tree rem = size_binop_loc (loc, TRUNC_MOD_EXPR, offs, bpu);
+  offs = size_binop_loc (loc, TRUNC_DIV_EXPR, offs, bpu);
+
+  tree s = fold_convert (size_type_node, TREE_OPERAND (node, 1));
+  s = size_binop_loc (loc, PLUS_EXPR, s, rem);
+  s = size_binop_loc (loc, CEIL_DIV_EXPR, s, bpu);
+  s = fold_convert (size_type_node, s);
+
+  *offset = offs;
+  *size = s;
+}
+
 /* Compute and return bounds for address of OBJ.  */
 static tree
 chkp_make_addressed_object_bounds (tree obj, gimple_stmt_iterator *iter)
@@ -3490,6 +3553,7 @@ chkp_make_addressed_object_bounds (tree obj, gimple_stmt_iterator *iter)
 
     case ARRAY_REF:
     case COMPONENT_REF:
+    case BIT_FIELD_REF:
       {
 	tree elt;
 	tree ptr;
@@ -3993,23 +4057,15 @@ chkp_process_stmt (gimple_stmt_iterator *iter, tree node,
 
     case BIT_FIELD_REF:
       {
-	tree offs, rem, bpu;
+	tree offset, size;
 
 	gcc_assert (!access_offs);
 	gcc_assert (!access_size);
 
-	bpu = fold_convert (size_type_node, bitsize_int (BITS_PER_UNIT));
-	offs = fold_convert (size_type_node, TREE_OPERAND (node, 2));
-	rem = size_binop_loc (loc, TRUNC_MOD_EXPR, offs, bpu);
-	offs = size_binop_loc (loc, TRUNC_DIV_EXPR, offs, bpu);
-
-	size = fold_convert (size_type_node, TREE_OPERAND (node, 1));
-        size = size_binop_loc (loc, PLUS_EXPR, size, rem);
-        size = size_binop_loc (loc, CEIL_DIV_EXPR, size, bpu);
-        size = fold_convert (size_type_node, size);
+	chkp_parse_bit_field_ref (node, loc, &offset, &size);
 
 	chkp_process_stmt (iter, TREE_OPERAND (node, 0), loc,
-			 dirflag, offs, size, safe);
+			   dirflag, offset, size, safe);
 	return;
       }
       break;
-- 
2.11.1

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Richard Biener]

On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>> marxin <mliska@suse.cz> writes:
>>
>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>> new file mode 100644
>>> index 00000000000..f9223db1b2d
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>> @@ -0,0 +1,4 @@
>>> +/* { dg-do compile { target { ! x32 } } } */
>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>
>> ... and again: make this x86-only.
>>
>>       Rainer
>>
>
> Thanks. I'm sending v2 of the patch.

Hmm, not sure why we should handle REAL_CST here explicitely for example.

Why not, instead of internal_error in the default: case do

  bounds = chkp_get_invalid_op_bounds ();

there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
a REAL_CST for example?

Richard.

> Martin

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Richard Biener]

On Thu, Mar 2, 2017 at 6:06 PM, marxin <mliska@suse.cz> wrote:
> gcc/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
>         PR ipa/79761
>         * tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
>         (chkp_find_bounds_1): Remove gcc_unreachable.
>
> gcc/testsuite/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
>         PR ipa/79761
>         * g++.dg/pr79761.C: New test.
> ---
>  gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
>  gcc/tree-chkp.c                |  3 +--
>  2 files changed, 35 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/pr79761.C
>
> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
> new file mode 100644
> index 00000000000..b1f92d2b036
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr79761.C
> @@ -0,0 +1,34 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
> +
> +struct Foo
> +{
> +  Foo() : a(1), b(1), c('a') {}
> +  int a;
> +  int b;
> +  char c;
> +};
> +
> +static Foo copy_foo(Foo) __attribute__((noinline, noclone));
> +
> +static Foo copy_foo(Foo A)
> +{
> +  return A;
> +}
> +
> +struct Bar : Foo
> +{
> +  Bar(Foo t) : Foo(copy_foo(t)) {}
> +};
> +
> +Foo F;
> +
> +int main (void)
> +{
> +  Bar B (F);
> +
> +  if (B.a != 1 || B.b != 1 || B.c != 'a')
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 3d497f51ed8..d5683b1b9cf 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
>  static tree
>  chkp_get_bound_for_parm (tree parm)
>  {
> -  tree decl = SSA_NAME_VAR (parm);
> +  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
>    tree bounds;
>
>    gcc_assert (TREE_CODE (decl) == PARM_DECL);
> @@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
>        break;
>
>      case PARM_DECL:
> -      gcc_unreachable ();
>        bounds = chkp_get_bound_for_parm (ptr_src);

But this is just useless work ... just do

      case PARM_DECL:
         /* Handled above but failed.  */
         break;

the SSA_NAME case is similarly redundantly calling chkp_get_registered_bounds.

Richard.

>        break;
>
> --
> 2.11.1
>
>

Re: [PATCH 5/5] Support BIT_FIELD_REF in MPX (PR ipa/79764).

[Richard Biener]

On Tue, Mar 7, 2017 at 3:49 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/07/2017 11:18 AM, Rainer Orth wrote:
>> marxin <mliska@suse.cz> writes:
>>
>>> diff --git a/gcc/testsuite/g++.dg/pr79764.C b/gcc/testsuite/g++.dg/pr79764.C
>>> new file mode 100644
>>> index 00000000000..47fb88da19b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr79764.C
>>> @@ -0,0 +1,12 @@
>>> +/* { dg-do compile { target { ! x32 } } } */
>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
>>
>> Same here.
>>
>>       Rainer
>>
>
> Thanks. I'm sending v2 of the patch.

Ok.

Richard.

> Martin

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

On 03/07/2017 03:53 PM, Richard Biener wrote:
> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>> marxin <mliska@suse.cz> writes:
>>>
>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>> new file mode 100644
>>>> index 00000000000..f9223db1b2d
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>> @@ -0,0 +1,4 @@
>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>
>>> ... and again: make this x86-only.
>>>
>>>       Rainer
>>>
>>
>> Thanks. I'm sending v2 of the patch.
> 
> Hmm, not sure why we should handle REAL_CST here explicitely for example.
> 
> Why not, instead of internal_error in the default: case do
> 
>   bounds = chkp_get_invalid_op_bounds ();

Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
security extension, I would be strict here in order to not handle something that can bypass
the checking.

> 
> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
> a REAL_CST for example?

It's called when setting bounds in a call expr:

#0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
#1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
#2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
#3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
#4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528

...

Martin

> 
> Richard.
> 
>> Martin

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

On 03/07/2017 03:57 PM, Richard Biener wrote:
> On Thu, Mar 2, 2017 at 6:06 PM, marxin <mliska@suse.cz> wrote:
>> gcc/ChangeLog:
>>
>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>
>>         PR ipa/79761
>>         * tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
>>         (chkp_find_bounds_1): Remove gcc_unreachable.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>
>>         PR ipa/79761
>>         * g++.dg/pr79761.C: New test.
>> ---
>>  gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
>>  gcc/tree-chkp.c                |  3 +--
>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>  create mode 100644 gcc/testsuite/g++.dg/pr79761.C
>>
>> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
>> new file mode 100644
>> index 00000000000..b1f92d2b036
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr79761.C
>> @@ -0,0 +1,34 @@
>> +/* { dg-do compile { target { ! x32 } } } */
>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>> +
>> +struct Foo
>> +{
>> +  Foo() : a(1), b(1), c('a') {}
>> +  int a;
>> +  int b;
>> +  char c;
>> +};
>> +
>> +static Foo copy_foo(Foo) __attribute__((noinline, noclone));
>> +
>> +static Foo copy_foo(Foo A)
>> +{
>> +  return A;
>> +}
>> +
>> +struct Bar : Foo
>> +{
>> +  Bar(Foo t) : Foo(copy_foo(t)) {}
>> +};
>> +
>> +Foo F;
>> +
>> +int main (void)
>> +{
>> +  Bar B (F);
>> +
>> +  if (B.a != 1 || B.b != 1 || B.c != 'a')
>> +    __builtin_abort ();
>> +
>> +  return 0;
>> +}
>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>> index 3d497f51ed8..d5683b1b9cf 100644
>> --- a/gcc/tree-chkp.c
>> +++ b/gcc/tree-chkp.c
>> @@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
>>  static tree
>>  chkp_get_bound_for_parm (tree parm)
>>  {
>> -  tree decl = SSA_NAME_VAR (parm);
>> +  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
>>    tree bounds;
>>
>>    gcc_assert (TREE_CODE (decl) == PARM_DECL);
>> @@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
>>        break;
>>
>>      case PARM_DECL:
>> -      gcc_unreachable ();
>>        bounds = chkp_get_bound_for_parm (ptr_src);
> 
> But this is just useless work ... just do
> 
>       case PARM_DECL:
>          /* Handled above but failed.  */
>          break;

Ok, let's return invalid bounds. Please see updated patch.

Martin

> 
> the SSA_NAME case is similarly redundantly calling chkp_get_registered_bounds.
> 
> Richard.
> 
>>        break;
>>
>> --
>> 2.11.1
>>
>>


[-- Attachment #2: 0002-Get-bounds-for-a-PARM_DECL-PR-ipa-79761-v3.patch --]
[-- Type: text/x-patch, Size: 1937 bytes --]

From feb00580b4f084ccd376a548d1121b1718a4806e Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 2 Mar 2017 18:06:39 +0100
Subject: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
	(chkp_find_bounds_1): Remove gcc_unreachable.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR ipa/79761
	* g++.dg/pr79761.C: New test.
---
 gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
 gcc/tree-chkp.c                |  4 ++--
 2 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr79761.C

diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
new file mode 100644
index 00000000000..a97325a1fc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr79761.C
@@ -0,0 +1,34 @@
+/* { dg-do compile { target { { i?86-*-* x86_64-*-* } && { ! x32 } } } } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
+
+struct Foo
+{
+  Foo() : a(1), b(1), c('a') {}
+  int a;
+  int b;
+  char c;
+};
+
+static Foo copy_foo(Foo) __attribute__((noinline, noclone));
+
+static Foo copy_foo(Foo A)
+{
+  return A;
+}
+
+struct Bar : Foo
+{
+  Bar(Foo t) : Foo(copy_foo(t)) {}
+};
+
+Foo F;
+
+int main (void)
+{
+  Bar B (F);
+
+  if (B.a != 1 || B.b != 1 || B.c != 'a')
+    __builtin_abort ();
+
+  return 0;
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 3d497f51ed8..75d8b5829d0 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3602,8 +3602,8 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
       break;
 
     case PARM_DECL:
-      gcc_unreachable ();
-      bounds = chkp_get_bound_for_parm (ptr_src);
+      /* Handled above but failed.  */
+      bounds = chkp_get_invalid_op_bounds ();
       break;
 
     case TARGET_MEM_REF:
-- 
2.11.1

Re: [PATCH 4/5] Disable -fcheck-pointer-bounds with sanitizers.

[Jeff Law]

On 03/06/2017 06:07 AM, marxin wrote:
> gcc/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
> 	PR target/65705
> 	PR target/69804
> 	* toplev.c (process_options): Disable -fcheck-pointer-bounds with
> 	sanitizers.
>
> gcc/testsuite/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
> 	PR target/65705
> 	PR target/69804
> 	* gcc.target/i386/pr71458.c: Update scanned pattern.
OK.
jeff

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Jeff Law]

On 03/07/2017 09:07 AM, Martin Liška wrote:
> On 03/07/2017 03:57 PM, Richard Biener wrote:
>> On Thu, Mar 2, 2017 at 6:06 PM, marxin <mliska@suse.cz> wrote:
>>> gcc/ChangeLog:
>>>
>>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/79761
>>>         * tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
>>>         (chkp_find_bounds_1): Remove gcc_unreachable.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/79761
>>>         * g++.dg/pr79761.C: New test.
>>> ---
>>>  gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
>>>  gcc/tree-chkp.c                |  3 +--
>>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>>  create mode 100644 gcc/testsuite/g++.dg/pr79761.C
>>>
>>> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
>>> new file mode 100644
>>> index 00000000000..b1f92d2b036
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr79761.C
>>> @@ -0,0 +1,34 @@
>>> +/* { dg-do compile { target { ! x32 } } } */
>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>> +
>>> +struct Foo
>>> +{
>>> +  Foo() : a(1), b(1), c('a') {}
>>> +  int a;
>>> +  int b;
>>> +  char c;
>>> +};
>>> +
>>> +static Foo copy_foo(Foo) __attribute__((noinline, noclone));
>>> +
>>> +static Foo copy_foo(Foo A)
>>> +{
>>> +  return A;
>>> +}
>>> +
>>> +struct Bar : Foo
>>> +{
>>> +  Bar(Foo t) : Foo(copy_foo(t)) {}
>>> +};
>>> +
>>> +Foo F;
>>> +
>>> +int main (void)
>>> +{
>>> +  Bar B (F);
>>> +
>>> +  if (B.a != 1 || B.b != 1 || B.c != 'a')
>>> +    __builtin_abort ();
>>> +
>>> +  return 0;
>>> +}
>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>> index 3d497f51ed8..d5683b1b9cf 100644
>>> --- a/gcc/tree-chkp.c
>>> +++ b/gcc/tree-chkp.c
>>> @@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
>>>  static tree
>>>  chkp_get_bound_for_parm (tree parm)
>>>  {
>>> -  tree decl = SSA_NAME_VAR (parm);
>>> +  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
>>>    tree bounds;
>>>
>>>    gcc_assert (TREE_CODE (decl) == PARM_DECL);
>>> @@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
>>>        break;
>>>
>>>      case PARM_DECL:
>>> -      gcc_unreachable ();
>>>        bounds = chkp_get_bound_for_parm (ptr_src);
>> But this is just useless work ... just do
>>
>>       case PARM_DECL:
>>          /* Handled above but failed.  */
>>          break;
> Ok, let's return invalid bounds. Please see updated patch.
>
> Martin
>
>> the SSA_NAME case is similarly redundantly calling chkp_get_registered_bounds.
>>
>> Richard.
>>
>>>        break;
>>>
>>> --
>>> 2.11.1
>>>
>>>
>
> 0002-Get-bounds-for-a-PARM_DECL-PR-ipa-79761-v3.patch
>
>
> From feb00580b4f084ccd376a548d1121b1718a4806e Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Thu, 2 Mar 2017 18:06:39 +0100
> Subject: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).
>
> gcc/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
> 	PR ipa/79761
> 	* tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
> 	(chkp_find_bounds_1): Remove gcc_unreachable.
>
> gcc/testsuite/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
> 	PR ipa/79761
> 	* g++.dg/pr79761.C: New test.
OK.  Sadly, I don't think any of these patches fix the P1 regressions we 
have for MPX.
jeff

Re: [PATCH 4/5] Disable -fcheck-pointer-bounds with sanitizers.

[Jakub Jelinek]

On Mon, Mar 06, 2017 at 02:07:37PM +0100, marxin wrote:
> 	PR target/65705
> 	PR target/69804
> 	* toplev.c (process_options): Disable -fcheck-pointer-bounds with
> 	sanitizers.

I can understand why it is disabled for -fsanitize=address or
-fsanitize=bounds, perhaps -fsanitize=threads too,
but don't understand why e.g. -fsanitize=shift or -fsanitize=unreachable
or -fsanitize=signed-integer-overflow or -fsanitize=leak (which is purely
a linking option) should affect it.

> +      const char *sanitizer_names[] = { "Address", "Undefined Behavior",
> +	"Leak", "Thread" };
> +      const int sanitizer_flags[] = { SANITIZE_ADDRESS, SANITIZE_UNDEFINED,
> +	SANITIZE_LEAK, SANITIZE_THREAD };

Even if there is a reason for that, there is also
SANITIZE_NONDEFAULT that is part of UB sanitizer, so if you can't
-fcheck-pointer-bounds with any parts of -fsanitize=undefined, then
likely it applies to others too.
For -fsanitize=bounds-stricts it surely applies though, if -fsanitize=bounds
can't be MPX instrumented.

> +      for (unsigned i = 0; i < sizeof (sanitizer_flags) / sizeof (int); i++)
> +	if (flag_sanitize & sanitizer_flags[i])
> +	  {
> +	    error_at (UNKNOWN_LOCATION,
> +		      "-fcheck-pointer-bounds is not supported with "
> +		      "%s Sanitizer", sanitizer_names[i]);

This is not i18n friendly, I think you just want to unroll the loop
by hand and use 4 (or just 3?) different error_at calls.

	Jakub

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Richard Biener]

On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/07/2017 03:53 PM, Richard Biener wrote:
>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>> marxin <mliska@suse.cz> writes:
>>>>
>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>> new file mode 100644
>>>>> index 00000000000..f9223db1b2d
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>> @@ -0,0 +1,4 @@
>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>
>>>> ... and again: make this x86-only.
>>>>
>>>>       Rainer
>>>>
>>>
>>> Thanks. I'm sending v2 of the patch.
>>
>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>
>> Why not, instead of internal_error in the default: case do
>>
>>   bounds = chkp_get_invalid_op_bounds ();
>
> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
> security extension, I would be strict here in order to not handle something that can bypass
> the checking.
>
>>
>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>> a REAL_CST for example?
>
> It's called when setting bounds in a call expr:
>
> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528

So this happens for calls with mismatched arguments?  I believe that

      if (BOUNDED_TYPE_P (type)
          || pass_by_reference (NULL, TYPE_MODE (type), type, true))
        new_args.safe_push (chkp_find_bounds (call_arg, gsi));

may be misguided given pass_by_reference is not exposed at GIMPLE and thus
for the pass_by_reference case it should fall back to
"chkp_get_invalid_op_bounds"
and thus eventually pass this down as a flag to chkp_find_bounds (or check
on call_arg again).

Note that here 'type' and call_arg may not match (asking for trouble).  Plus
'fntype' is computed bogously (should use gimple_call_fntype).

It later does sth like

  /* For direct calls fndecl is replaced with instrumented version.  */
  if (fndecl)
    {
      tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
      gimple_call_set_fndecl (new_call, new_decl);
      /* In case of a type cast we should modify used function
         type instead of using type of new fndecl.  */
      if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
        {
          tree type = gimple_call_fntype (call);
          type = chkp_copy_function_type_adding_bounds (type);
          gimple_call_set_fntype (new_call, type);
        }
      else
        gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));

but obviously if gimple_call_fntype and fndecl don't match cloning
the mismatched decl isn't of very much help ... (it may just generate
more mismatches with mismatching bounds...).

> ...
>
> Martin
>
>>
>> Richard.
>>
>>> Martin
>

Re: [PATCH 2/5] Get bounds for a PARM_DECL (PR ipa/79761).

[Richard Biener]

On Tue, Mar 7, 2017 at 5:07 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/07/2017 03:57 PM, Richard Biener wrote:
>> On Thu, Mar 2, 2017 at 6:06 PM, marxin <mliska@suse.cz> wrote:
>>> gcc/ChangeLog:
>>>
>>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/79761
>>>         * tree-chkp.c (chkp_get_bound_for_parm): Get bounds for a param.
>>>         (chkp_find_bounds_1): Remove gcc_unreachable.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/79761
>>>         * g++.dg/pr79761.C: New test.
>>> ---
>>>  gcc/testsuite/g++.dg/pr79761.C | 34 ++++++++++++++++++++++++++++++++++
>>>  gcc/tree-chkp.c                |  3 +--
>>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>>  create mode 100644 gcc/testsuite/g++.dg/pr79761.C
>>>
>>> diff --git a/gcc/testsuite/g++.dg/pr79761.C b/gcc/testsuite/g++.dg/pr79761.C
>>> new file mode 100644
>>> index 00000000000..b1f92d2b036
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr79761.C
>>> @@ -0,0 +1,34 @@
>>> +/* { dg-do compile { target { ! x32 } } } */
>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>> +
>>> +struct Foo
>>> +{
>>> +  Foo() : a(1), b(1), c('a') {}
>>> +  int a;
>>> +  int b;
>>> +  char c;
>>> +};
>>> +
>>> +static Foo copy_foo(Foo) __attribute__((noinline, noclone));
>>> +
>>> +static Foo copy_foo(Foo A)
>>> +{
>>> +  return A;
>>> +}
>>> +
>>> +struct Bar : Foo
>>> +{
>>> +  Bar(Foo t) : Foo(copy_foo(t)) {}
>>> +};
>>> +
>>> +Foo F;
>>> +
>>> +int main (void)
>>> +{
>>> +  Bar B (F);
>>> +
>>> +  if (B.a != 1 || B.b != 1 || B.c != 'a')
>>> +    __builtin_abort ();
>>> +
>>> +  return 0;
>>> +}
>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>> index 3d497f51ed8..d5683b1b9cf 100644
>>> --- a/gcc/tree-chkp.c
>>> +++ b/gcc/tree-chkp.c
>>> @@ -2353,7 +2353,7 @@ chkp_get_next_bounds_parm (tree parm)
>>>  static tree
>>>  chkp_get_bound_for_parm (tree parm)
>>>  {
>>> -  tree decl = SSA_NAME_VAR (parm);
>>> +  tree decl = TREE_CODE (parm) == PARM_DECL ? parm : SSA_NAME_VAR (parm);
>>>    tree bounds;
>>>
>>>    gcc_assert (TREE_CODE (decl) == PARM_DECL);
>>> @@ -3602,7 +3602,6 @@ chkp_find_bounds_1 (tree ptr, tree ptr_src, gimple_stmt_iterator *iter)
>>>        break;
>>>
>>>      case PARM_DECL:
>>> -      gcc_unreachable ();
>>>        bounds = chkp_get_bound_for_parm (ptr_src);
>>
>> But this is just useless work ... just do
>>
>>       case PARM_DECL:
>>          /* Handled above but failed.  */
>>          break;
>
> Ok, let's return invalid bounds. Please see updated patch.

Ok.

> Martin
>
>>
>> the SSA_NAME case is similarly redundantly calling chkp_get_registered_bounds.
>>
>> Richard.
>>
>>>        break;
>>>
>>> --
>>> 2.11.1
>>>
>>>
>

Re: [PATCH 3/5] Fix ICE in tree-chkp-opt.c (PR tree-optimization/79631).

[Martin Liška]

PING^1:

On 03/02/2017 06:15 PM, marxin wrote:
> gcc/ChangeLog:
> 
> 2017-03-06  Martin Liska  <mliska@suse.cz>
> 
> 	PR tree-optimization/79631
> 	* tree-chkp-opt.c (chkp_is_constant_addr): Call
> 	tree_int_cst_sign_bit just for INTEGER constants.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-03-06  Martin Liska  <mliska@suse.cz>
> 
> 	PR tree-optimization/79631
> 	* gcc.target/i386/mpx/pr79631.c: New test.
> ---
>  gcc/testsuite/gcc.target/i386/mpx/pr79631.c | 15 +++++++++++++++
>  gcc/tree-chkp-opt.c                         |  3 ++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79631.c
> 
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79631.c b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
> new file mode 100644
> index 00000000000..075d46b835f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -O2" } */
> +
> +typedef struct { int _mp_size; } mpz_t[1];
> +int a, b;
> +void fn1()
> +{
> +  mpz_t c[1][b];
> +  for (;;) {
> +      int d = 0 >= 0 ? 0 == 0 ? c[0][0]->_mp_size ? -1 : 0 : 0 : 0,
> +	  e = 0 >= 0 ? 0 == 0 ? c[1][1]->_mp_size ? -1 : 0 : 0 : 0;
> +      if (d != e)
> +	a++;
> +  }
> +}
> diff --git a/gcc/tree-chkp-opt.c b/gcc/tree-chkp-opt.c
> index ebe05459773..286f7853921 100644
> --- a/gcc/tree-chkp-opt.c
> +++ b/gcc/tree-chkp-opt.c
> @@ -241,7 +241,8 @@ chkp_is_constant_addr (const address_t &addr, int *sign)
>      return false;
>    else if (integer_zerop (addr.pol[0].cst))
>      *sign = 0;
> -  else if  (tree_int_cst_sign_bit (addr.pol[0].cst))
> +  else if (TREE_CODE (addr.pol[0].cst) == INTEGER_CST
> +	   && tree_int_cst_sign_bit (addr.pol[0].cst))
>      *sign = -1;
>    else
>      *sign = 1;
> 

Re: [PATCH 4/5] Disable -fcheck-pointer-bounds with sanitizers.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

On 03/07/2017 05:21 PM, Jakub Jelinek wrote:
> On Mon, Mar 06, 2017 at 02:07:37PM +0100, marxin wrote:
>> 	PR target/65705
>> 	PR target/69804
>> 	* toplev.c (process_options): Disable -fcheck-pointer-bounds with
>> 	sanitizers.
> 
> I can understand why it is disabled for -fsanitize=address or
> -fsanitize=bounds, perhaps -fsanitize=threads too,
> but don't understand why e.g. -fsanitize=shift or -fsanitize=unreachable
> or -fsanitize=signed-integer-overflow or -fsanitize=leak (which is purely
> a linking option) should affect it.
> 
>> +      const char *sanitizer_names[] = { "Address", "Undefined Behavior",
>> +	"Leak", "Thread" };
>> +      const int sanitizer_flags[] = { SANITIZE_ADDRESS, SANITIZE_UNDEFINED,
>> +	SANITIZE_LEAK, SANITIZE_THREAD };
> 
> Even if there is a reason for that, there is also
> SANITIZE_NONDEFAULT that is part of UB sanitizer, so if you can't
> -fcheck-pointer-bounds with any parts of -fsanitize=undefined, then
> likely it applies to others too.
> For -fsanitize=bounds-stricts it surely applies though, if -fsanitize=bounds
> can't be MPX instrumented.
> 
>> +      for (unsigned i = 0; i < sizeof (sanitizer_flags) / sizeof (int); i++)
>> +	if (flag_sanitize & sanitizer_flags[i])
>> +	  {
>> +	    error_at (UNKNOWN_LOCATION,
>> +		      "-fcheck-pointer-bounds is not supported with "
>> +		      "%s Sanitizer", sanitizer_names[i]);
> 
> This is not i18n friendly, I think you just want to unroll the loop
> by hand and use 4 (or just 3?) different error_at calls.
> 
> 	Jakub
> 

Thanks for review, I fixed both in patch that I'm going to install.

Martin

[-- Attachment #2: 0001-Disable-fcheck-pointer-bounds-with-sanitizers.patch --]
[-- Type: text/x-patch, Size: 2756 bytes --]

From a6c3066fb95b521d4b9ca396c115cbe32fab72ee Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 6 Mar 2017 14:07:37 +0100
Subject: [PATCH] Disable -fcheck-pointer-bounds with sanitizers.

gcc/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/65705
	PR target/69804
	* toplev.c (process_options): Disable -fcheck-pointer-bounds with
	sanitizers.

gcc/testsuite/ChangeLog:

2017-03-06  Martin Liska  <mliska@suse.cz>

	PR target/65705
	PR target/69804
	* gcc.target/i386/pr71458.c: Update scanned pattern.
---
 gcc/testsuite/gcc.target/i386/pr71458.c |  2 +-
 gcc/toplev.c                            | 32 ++++++++++++++++++++------------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/gcc/testsuite/gcc.target/i386/pr71458.c b/gcc/testsuite/gcc.target/i386/pr71458.c
index 27e7764b5a0..2faf6bb9391 100644
--- a/gcc/testsuite/gcc.target/i386/pr71458.c
+++ b/gcc/testsuite/gcc.target/i386/pr71458.c
@@ -1,6 +1,6 @@
 /* { dg-do compile { target { ! x32 } } } */
 /* { dg-options "-fcheck-pointer-bounds -mmpx -fsanitize=bounds" } */
-/* { dg-error "-fcheck-pointer-bounds is not supported with -fsanitize=bounds" "" { target *-*-* } 0 } */
+/* { dg-error "-fcheck-pointer-bounds is not supported with Undefined Behavior Sanitizer" "" { target *-*-* } 0 } */
 
 enum {} a[0];
 void fn1(int);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index beb581aba55..6a7e4fbdffb 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1274,22 +1274,30 @@ process_options (void)
 	  flag_check_pointer_bounds = 0;
 	}
 
-      if (flag_sanitize & SANITIZE_ADDRESS)
+      if (flag_sanitize)
 	{
-	  error_at (UNKNOWN_LOCATION,
-		    "-fcheck-pointer-bounds is not supported with "
-		    "Address Sanitizer");
-	  flag_check_pointer_bounds = 0;
-	}
+	  if (flag_sanitize & SANITIZE_ADDRESS)
+	    error_at (UNKNOWN_LOCATION,
+		      "-fcheck-pointer-bounds is not supported with "
+		      "Address Sanitizer");
+
+	  if (flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT))
+	    error_at (UNKNOWN_LOCATION,
+		      "-fcheck-pointer-bounds is not supported with "
+		      "Undefined Behavior Sanitizer");
+
+	  if (flag_sanitize & SANITIZE_LEAK)
+	    error_at (UNKNOWN_LOCATION,
+		      "-fcheck-pointer-bounds is not supported with "
+		      "Leak Sanitizer");
+
+	  if (flag_sanitize & SANITIZE_THREAD)
+	    error_at (UNKNOWN_LOCATION,
+		      "-fcheck-pointer-bounds is not supported with "
+		      "Thread Sanitizer");
 
-      if (flag_sanitize & SANITIZE_BOUNDS)
-	{
-	  error_at (UNKNOWN_LOCATION,
-		    "-fcheck-pointer-bounds is not supported with "
-		    "-fsanitize=bounds");
 	  flag_check_pointer_bounds = 0;
 	}
-
     }
 
   /* One region RA really helps to decrease the code size.  */
-- 
2.11.1

Re: [PATCH 3/5] Fix ICE in tree-chkp-opt.c (PR tree-optimization/79631).

[Richard Biener]

On Thu, Mar 2, 2017 at 6:15 PM, marxin <mliska@suse.cz> wrote:
> gcc/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
>         PR tree-optimization/79631
>         * tree-chkp-opt.c (chkp_is_constant_addr): Call
>         tree_int_cst_sign_bit just for INTEGER constants.
>
> gcc/testsuite/ChangeLog:
>
> 2017-03-06  Martin Liska  <mliska@suse.cz>
>
>         PR tree-optimization/79631
>         * gcc.target/i386/mpx/pr79631.c: New test.
> ---
>  gcc/testsuite/gcc.target/i386/mpx/pr79631.c | 15 +++++++++++++++
>  gcc/tree-chkp-opt.c                         |  3 ++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79631.c
>
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79631.c b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
> new file mode 100644
> index 00000000000..075d46b835f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target { ! x32 } } } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx -O2" } */
> +
> +typedef struct { int _mp_size; } mpz_t[1];
> +int a, b;
> +void fn1()
> +{
> +  mpz_t c[1][b];
> +  for (;;) {
> +      int d = 0 >= 0 ? 0 == 0 ? c[0][0]->_mp_size ? -1 : 0 : 0 : 0,
> +         e = 0 >= 0 ? 0 == 0 ? c[1][1]->_mp_size ? -1 : 0 : 0 : 0;
> +      if (d != e)
> +       a++;
> +  }
> +}
> diff --git a/gcc/tree-chkp-opt.c b/gcc/tree-chkp-opt.c
> index ebe05459773..286f7853921 100644
> --- a/gcc/tree-chkp-opt.c
> +++ b/gcc/tree-chkp-opt.c
> @@ -241,7 +241,8 @@ chkp_is_constant_addr (const address_t &addr, int *sign)
>      return false;
>    else if (integer_zerop (addr.pol[0].cst))
>      *sign = 0;
> -  else if  (tree_int_cst_sign_bit (addr.pol[0].cst))
> +  else if (TREE_CODE (addr.pol[0].cst) == INTEGER_CST
> +          && tree_int_cst_sign_bit (addr.pol[0].cst))
>      *sign = -1;
>    else
>      *sign = 1;

It looks like it assumes sign == 1 else and thus there likely should be

  else if (TREE_CODE (addr.pol[0].cst) != INTEGER_CST)
    return false;
  else if (integer_zerop ...)

to handle &foo I guess.

Ok with that change.

Richard.

> --
> 2.11.1
>
>

Re: [PATCH 3/5] Fix ICE in tree-chkp-opt.c (PR tree-optimization/79631).

[Martin Liška]

On 03/09/2017 11:04 AM, Richard Biener wrote:
> On Thu, Mar 2, 2017 at 6:15 PM, marxin <mliska@suse.cz> wrote:
>> gcc/ChangeLog:
>>
>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>
>>         PR tree-optimization/79631
>>         * tree-chkp-opt.c (chkp_is_constant_addr): Call
>>         tree_int_cst_sign_bit just for INTEGER constants.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-03-06  Martin Liska  <mliska@suse.cz>
>>
>>         PR tree-optimization/79631
>>         * gcc.target/i386/mpx/pr79631.c: New test.
>> ---
>>  gcc/testsuite/gcc.target/i386/mpx/pr79631.c | 15 +++++++++++++++
>>  gcc/tree-chkp-opt.c                         |  3 ++-
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>  create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79631.c
>>
>> diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79631.c b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
>> new file mode 100644
>> index 00000000000..075d46b835f
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/mpx/pr79631.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile { target { ! x32 } } } */
>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -O2" } */
>> +
>> +typedef struct { int _mp_size; } mpz_t[1];
>> +int a, b;
>> +void fn1()
>> +{
>> +  mpz_t c[1][b];
>> +  for (;;) {
>> +      int d = 0 >= 0 ? 0 == 0 ? c[0][0]->_mp_size ? -1 : 0 : 0 : 0,
>> +         e = 0 >= 0 ? 0 == 0 ? c[1][1]->_mp_size ? -1 : 0 : 0 : 0;
>> +      if (d != e)
>> +       a++;
>> +  }
>> +}
>> diff --git a/gcc/tree-chkp-opt.c b/gcc/tree-chkp-opt.c
>> index ebe05459773..286f7853921 100644
>> --- a/gcc/tree-chkp-opt.c
>> +++ b/gcc/tree-chkp-opt.c
>> @@ -241,7 +241,8 @@ chkp_is_constant_addr (const address_t &addr, int *sign)
>>      return false;
>>    else if (integer_zerop (addr.pol[0].cst))
>>      *sign = 0;
>> -  else if  (tree_int_cst_sign_bit (addr.pol[0].cst))
>> +  else if (TREE_CODE (addr.pol[0].cst) == INTEGER_CST
>> +          && tree_int_cst_sign_bit (addr.pol[0].cst))
>>      *sign = -1;
>>    else
>>      *sign = 1;
> 
> It looks like it assumes sign == 1 else and thus there likely should be

Yep, nice note.
Thanks for review.

Martin

> 
>   else if (TREE_CODE (addr.pol[0].cst) != INTEGER_CST)
>     return false;
>   else if (integer_zerop ...)
> 
> to handle &foo I guess.
> 
> Ok with that change.
> 
> Richard.
> 
>> --
>> 2.11.1
>>
>>

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

On 03/08/2017 12:00 PM, Richard Biener wrote:
> On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 03/07/2017 03:53 PM, Richard Biener wrote:
>>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>>> marxin <mliska@suse.cz> writes:
>>>>>
>>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>>> new file mode 100644
>>>>>> index 00000000000..f9223db1b2d
>>>>>> --- /dev/null
>>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>>> @@ -0,0 +1,4 @@
>>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>>
>>>>> ... and again: make this x86-only.
>>>>>
>>>>>       Rainer
>>>>>
>>>>
>>>> Thanks. I'm sending v2 of the patch.
>>>
>>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>>
>>> Why not, instead of internal_error in the default: case do
>>>
>>>   bounds = chkp_get_invalid_op_bounds ();
>>
>> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
>> security extension, I would be strict here in order to not handle something that can bypass
>> the checking.
>>
>>>
>>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>>> a REAL_CST for example?
>>
>> It's called when setting bounds in a call expr:
>>
>> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
>> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
>> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
>> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
>> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528

Yes, as you pointed out, we've got 2 issues here:

> 
> So this happens for calls with mismatched arguments?  I believe that
> 
>       if (BOUNDED_TYPE_P (type)
>           || pass_by_reference (NULL, TYPE_MODE (type), type, true))
>         new_args.safe_push (chkp_find_bounds (call_arg, gsi));
> 
> may be misguided given pass_by_reference is not exposed at GIMPLE and thus
> for the pass_by_reference case it should fall back to
> "chkp_get_invalid_op_bounds"
> and thus eventually pass this down as a flag to chkp_find_bounds (or check
> on call_arg again).

Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to

(gdb) p debug_tree(call_arg)
 <ssa_name 0x7ffff69d5120
    type <pointer_type 0x7ffff69c9e70
        type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
            sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836> unit size <var_decl 0x7ffff69d33f0 D.1837>
            align 32 symtab 0 alias set -1 structural equality domain <integer_type 0x7ffff69c9dc8>
            pointer_to_this <pointer_type 0x7ffff69c9e70>>
        unsigned DI
        size <integer_cst 0x7ffff6876cd8 constant 64>
        unit size <integer_cst 0x7ffff6876cf0 constant 8>
        align 64 symtab 0 alias set -1 structural equality>
    visited
    def_stmt x.2_7 = x.1_20;
    version 7>

where bounds result in:
  _18 = _6 * 4;
  x.1_20 = __builtin_alloca_with_align (_18, 32);
  __bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);

which is consider fine. I don't understand how can be that broken on gimple as it's not exposed
by GIMPLE?

> 
> Note that here 'type' and call_arg may not match (asking for trouble).  Plus
> 'fntype' is computed bogously (should use gimple_call_fntype).
> 
> It later does sth like
> 
>   /* For direct calls fndecl is replaced with instrumented version.  */
>   if (fndecl)
>     {
>       tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
>       gimple_call_set_fndecl (new_call, new_decl);
>       /* In case of a type cast we should modify used function
>          type instead of using type of new fndecl.  */
>       if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
>         {
>           tree type = gimple_call_fntype (call);
>           type = chkp_copy_function_type_adding_bounds (type);
>           gimple_call_set_fntype (new_call, type);
>         }
>       else
>         gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));
> 
> but obviously if gimple_call_fntype and fndecl don't match cloning
> the mismatched decl isn't of very much help ... (it may just generate
> more mismatches with mismatching bounds...).

That second issue should be probably fixed by comparing each formal and actual arguments,
where for a mismatch an invalid bounds should be returned? Am I right?

Martin

> 
>> ...
>>
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>

[PATCH 6/N] Do not warn -Wsuggest-attribute=noreturn for main.chkp (PR, middle-end/78339).

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Hello.

Following patch fixes issue by checking original declaration instead
of instrumentation clone.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin

[-- Attachment #2: 0001-Do-not-warn-Wsuggest-attribute-noreturn-for-main.chk.patch --]
[-- Type: text/x-patch, Size: 1992 bytes --]

From 1235ced464fa990eaa4157ea216aeac7e4023b06 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 7 Mar 2017 16:27:40 +0100
Subject: [PATCH] Do not warn -Wsuggest-attribute=noreturn for main.chkp (PR
 middle-end/78339).

gcc/ChangeLog:

2017-03-07  Martin Liska  <mliska@suse.cz>

	PR middle-end/78339
	* ipa-pure-const.c (warn_function_noreturn): If the declarations
	is a CHKP clone, use original declaration.

gcc/testsuite/ChangeLog:

2017-03-07  Martin Liska  <mliska@suse.cz>

	PR middle-end/78339
	* gcc.target/i386/mpx/pr78339.c: New test.
---
 gcc/ipa-pure-const.c                        | 8 +++++++-
 gcc/testsuite/gcc.target/i386/mpx/pr78339.c | 5 +++++
 2 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr78339.c

diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 5cc2002d024..5c6f775c4ac 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -218,11 +218,17 @@ warn_function_const (tree decl, bool known_finite)
 static void
 warn_function_noreturn (tree decl)
 {
+  tree original_decl = decl;
+
+  cgraph_node *node = cgraph_node::get (decl);
+  if (node->instrumentation_clone)
+    decl = node->instrumented_version->decl;
+
   static hash_set<tree> *warned_about;
   if (!lang_hooks.missing_noreturn_ok_p (decl)
       && targetm.warn_func_return (decl))
     warned_about 
-      = suggest_attribute (OPT_Wsuggest_attribute_noreturn, decl,
+      = suggest_attribute (OPT_Wsuggest_attribute_noreturn, original_decl,
 			   true, warned_about, "noreturn");
 }
 
diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr78339.c b/gcc/testsuite/gcc.target/i386/mpx/pr78339.c
new file mode 100644
index 00000000000..3dd04240e8c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr78339.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -Wsuggest-attribute=noreturn" } */
+
+extern _Noreturn void exit (int);
+int main (void) { exit (1); }
-- 
2.11.1

[PATCH] MPX: Fix option handling.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Hello.

This is follow-up patch which I agreed on with Jakub.
It enables CHKP with LSAN and majority of UBSAN options.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin

[-- Attachment #2: 0001-MPX-Fix-option-handling.patch --]
[-- Type: text/x-patch, Size: 3655 bytes --]

From a410d5e4e028d34dc00b4aa637cdcd3986b971d8 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 10 Mar 2017 11:05:27 +0100
Subject: [PATCH] MPX: Fix option handling.

gcc/ChangeLog:

2017-03-10  Martin Liska  <mliska@suse.cz>

        PR target/65705
        PR target/69804
	* toplev.c (process_options): Enable MPX with LSAN and UBSAN
	(except -fsanitize=bounds and -fsanitize=bounds-strict).
	* tree-chkp.c (chkp_walk_pointer_assignments): Verify that
	FIELD != NULL.
---
 gcc/testsuite/gcc.target/i386/pr71458.c |  2 +-
 gcc/toplev.c                            | 47 +++++++++++++++++++--------------
 gcc/tree-chkp.c                         |  2 +-
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/gcc/testsuite/gcc.target/i386/pr71458.c b/gcc/testsuite/gcc.target/i386/pr71458.c
index 2faf6bb9391..27e7764b5a0 100644
--- a/gcc/testsuite/gcc.target/i386/pr71458.c
+++ b/gcc/testsuite/gcc.target/i386/pr71458.c
@@ -1,6 +1,6 @@
 /* { dg-do compile { target { ! x32 } } } */
 /* { dg-options "-fcheck-pointer-bounds -mmpx -fsanitize=bounds" } */
-/* { dg-error "-fcheck-pointer-bounds is not supported with Undefined Behavior Sanitizer" "" { target *-*-* } 0 } */
+/* { dg-error "-fcheck-pointer-bounds is not supported with -fsanitize=bounds" "" { target *-*-* } 0 } */
 
 enum {} a[0];
 void fn1(int);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 6a7e4fbdffb..38bc33e007e 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1274,27 +1274,34 @@ process_options (void)
 	  flag_check_pointer_bounds = 0;
 	}
 
-      if (flag_sanitize)
+      if (flag_sanitize & SANITIZE_BOUNDS_STRICT)
 	{
-	  if (flag_sanitize & SANITIZE_ADDRESS)
-	    error_at (UNKNOWN_LOCATION,
-		      "-fcheck-pointer-bounds is not supported with "
-		      "Address Sanitizer");
-
-	  if (flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT))
-	    error_at (UNKNOWN_LOCATION,
-		      "-fcheck-pointer-bounds is not supported with "
-		      "Undefined Behavior Sanitizer");
-
-	  if (flag_sanitize & SANITIZE_LEAK)
-	    error_at (UNKNOWN_LOCATION,
-		      "-fcheck-pointer-bounds is not supported with "
-		      "Leak Sanitizer");
-
-	  if (flag_sanitize & SANITIZE_THREAD)
-	    error_at (UNKNOWN_LOCATION,
-		      "-fcheck-pointer-bounds is not supported with "
-		      "Thread Sanitizer");
+	  error_at (UNKNOWN_LOCATION,
+		    "-fcheck-pointer-bounds is not supported with "
+		    "-fsanitize=bounds-strict");
+	  flag_check_pointer_bounds = 0;
+	}
+      else if (flag_sanitize & SANITIZE_BOUNDS)
+	{
+	  error_at (UNKNOWN_LOCATION,
+		    "-fcheck-pointer-bounds is not supported with "
+		    "-fsanitize=bounds");
+	  flag_check_pointer_bounds = 0;
+	}
+
+      if (flag_sanitize & SANITIZE_ADDRESS)
+	{
+	  error_at (UNKNOWN_LOCATION,
+		    "-fcheck-pointer-bounds is not supported with "
+		    "Address Sanitizer");
+	  flag_check_pointer_bounds = 0;
+	}
+
+      if (flag_sanitize & SANITIZE_THREAD)
+	{
+	  error_at (UNKNOWN_LOCATION,
+		    "-fcheck-pointer-bounds is not supported with "
+		    "Thread Sanitizer");
 
 	  flag_check_pointer_bounds = 0;
 	}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index c057b977342..fdb95ee5cc6 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3804,7 +3804,7 @@ chkp_walk_pointer_assignments (tree lhs, tree rhs, void *arg,
 
 	  FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (rhs), cnt, field, val)
 	    {
-	      if (chkp_type_has_pointer (TREE_TYPE (field)))
+	      if (field && chkp_type_has_pointer (TREE_TYPE (field)))
 		{
 		  tree lhs_field = chkp_build_component_ref (lhs, field);
 		  chkp_walk_pointer_assignments (lhs_field, val, arg, handler);
-- 
2.11.1

Re: [PATCH] MPX: Fix option handling.

[Jakub Jelinek]

On Fri, Mar 10, 2017 at 02:09:20PM +0100, Martin Liška wrote:
> Hello.
> 
> This is follow-up patch which I agreed on with Jakub.
> It enables CHKP with LSAN and majority of UBSAN options.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Martin

> >From a410d5e4e028d34dc00b4aa637cdcd3986b971d8 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Fri, 10 Mar 2017 11:05:27 +0100
> Subject: [PATCH] MPX: Fix option handling.
> 
> gcc/ChangeLog:
> 
> 2017-03-10  Martin Liska  <mliska@suse.cz>
> 
>         PR target/65705
>         PR target/69804
> 	* toplev.c (process_options): Enable MPX with LSAN and UBSAN
> 	(except -fsanitize=bounds and -fsanitize=bounds-strict).

Please avoid the ()s, that is confusing with ()s used to surround
function etc. names.  Just use UBSAN,
	except ... strict.

> 	* tree-chkp.c (chkp_walk_pointer_assignments): Verify that
> 	FIELD != NULL.

> +	  error_at (UNKNOWN_LOCATION,
> +		    "-fcheck-pointer-bounds is not supported with "
> +		    "-fsanitize=bounds-strict");
> +	  flag_check_pointer_bounds = 0;

Given the recent i18n discussions, perhaps this ought to be
%<-fcheck-pointer-bounds%> and %<-fsanitize=bounds-strict%> and similarly
elsewhere (of course not for Address/Thread Sanitizer words).

Ok for trunk either way.

	Jakub

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Richard Biener]

On Thu, Mar 9, 2017 at 12:40 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/08/2017 12:00 PM, Richard Biener wrote:
>> On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 03/07/2017 03:53 PM, Richard Biener wrote:
>>>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>>>> marxin <mliska@suse.cz> writes:
>>>>>>
>>>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..f9223db1b2d
>>>>>>> --- /dev/null
>>>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>> @@ -0,0 +1,4 @@
>>>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>>>
>>>>>> ... and again: make this x86-only.
>>>>>>
>>>>>>       Rainer
>>>>>>
>>>>>
>>>>> Thanks. I'm sending v2 of the patch.
>>>>
>>>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>>>
>>>> Why not, instead of internal_error in the default: case do
>>>>
>>>>   bounds = chkp_get_invalid_op_bounds ();
>>>
>>> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
>>> security extension, I would be strict here in order to not handle something that can bypass
>>> the checking.
>>>
>>>>
>>>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>>>> a REAL_CST for example?
>>>
>>> It's called when setting bounds in a call expr:
>>>
>>> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
>>> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
>>> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
>>> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
>>> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528
>
> Yes, as you pointed out, we've got 2 issues here:
>
>>
>> So this happens for calls with mismatched arguments?  I believe that
>>
>>       if (BOUNDED_TYPE_P (type)
>>           || pass_by_reference (NULL, TYPE_MODE (type), type, true))
>>         new_args.safe_push (chkp_find_bounds (call_arg, gsi));
>>
>> may be misguided given pass_by_reference is not exposed at GIMPLE and thus
>> for the pass_by_reference case it should fall back to
>> "chkp_get_invalid_op_bounds"
>> and thus eventually pass this down as a flag to chkp_find_bounds (or check
>> on call_arg again).
>
> Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to
>
> (gdb) p debug_tree(call_arg)
>  <ssa_name 0x7ffff69d5120
>     type <pointer_type 0x7ffff69c9e70
>         type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
>             sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836> unit size <var_decl 0x7ffff69d33f0 D.1837>
>             align 32 symtab 0 alias set -1 structural equality domain <integer_type 0x7ffff69c9dc8>
>             pointer_to_this <pointer_type 0x7ffff69c9e70>>
>         unsigned DI
>         size <integer_cst 0x7ffff6876cd8 constant 64>
>         unit size <integer_cst 0x7ffff6876cf0 constant 8>
>         align 64 symtab 0 alias set -1 structural equality>
>     visited
>     def_stmt x.2_7 = x.1_20;
>     version 7>
>
> where bounds result in:
>   _18 = _6 * 4;
>   x.1_20 = __builtin_alloca_with_align (_18, 32);
>   __bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);
>
> which is consider fine. I don't understand how can be that broken on gimple as it's not exposed
> by GIMPLE?

Not sure how this relates to my question.

>>
>> Note that here 'type' and call_arg may not match (asking for trouble).  Plus
>> 'fntype' is computed bogously (should use gimple_call_fntype).
>>
>> It later does sth like
>>
>>   /* For direct calls fndecl is replaced with instrumented version.  */
>>   if (fndecl)
>>     {
>>       tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
>>       gimple_call_set_fndecl (new_call, new_decl);
>>       /* In case of a type cast we should modify used function
>>          type instead of using type of new fndecl.  */
>>       if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
>>         {
>>           tree type = gimple_call_fntype (call);
>>           type = chkp_copy_function_type_adding_bounds (type);
>>           gimple_call_set_fntype (new_call, type);
>>         }
>>       else
>>         gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));
>>
>> but obviously if gimple_call_fntype and fndecl don't match cloning
>> the mismatched decl isn't of very much help ... (it may just generate
>> more mismatches with mismatching bounds...).
>
> That second issue should be probably fixed by comparing each formal and actual arguments,
> where for a mismatch an invalid bounds should be returned? Am I right?

It's hard to say what to do for calls through incompatible tyes.  I'd
say it should use
the not instrumented copy (which there may be none if the function was
static and we
didn't need a clone?).  After all we're likely passing garbage to the
function anyway.

Richard.

> Martin
>
>>
>>> ...
>>>
>>> Martin
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Martin
>>>
>

Re: [PATCH 6/N] Do not warn -Wsuggest-attribute=noreturn for main.chkp (PR, middle-end/78339).

[Richard Biener]

On Thu, Mar 9, 2017 at 3:19 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> Following patch fixes issue by checking original declaration instead
> of instrumentation clone.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Ok.

RIchard.

> Martin

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

On 03/13/2017 01:28 PM, Richard Biener wrote:
> On Thu, Mar 9, 2017 at 12:40 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 03/08/2017 12:00 PM, Richard Biener wrote:
>>> On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
>>>> On 03/07/2017 03:53 PM, Richard Biener wrote:
>>>>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>>>>> marxin <mliska@suse.cz> writes:
>>>>>>>
>>>>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>>> new file mode 100644
>>>>>>>> index 00000000000..f9223db1b2d
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>>> @@ -0,0 +1,4 @@
>>>>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>>>>
>>>>>>> ... and again: make this x86-only.
>>>>>>>
>>>>>>>       Rainer
>>>>>>>
>>>>>>
>>>>>> Thanks. I'm sending v2 of the patch.
>>>>>
>>>>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>>>>
>>>>> Why not, instead of internal_error in the default: case do
>>>>>
>>>>>   bounds = chkp_get_invalid_op_bounds ();
>>>>
>>>> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
>>>> security extension, I would be strict here in order to not handle something that can bypass
>>>> the checking.
>>>>
>>>>>
>>>>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>>>>> a REAL_CST for example?
>>>>
>>>> It's called when setting bounds in a call expr:
>>>>
>>>> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
>>>> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
>>>> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
>>>> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
>>>> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528
>>
>> Yes, as you pointed out, we've got 2 issues here:
>>
>>>
>>> So this happens for calls with mismatched arguments?  I believe that
>>>
>>>       if (BOUNDED_TYPE_P (type)
>>>           || pass_by_reference (NULL, TYPE_MODE (type), type, true))
>>>         new_args.safe_push (chkp_find_bounds (call_arg, gsi));
>>>
>>> may be misguided given pass_by_reference is not exposed at GIMPLE and thus
>>> for the pass_by_reference case it should fall back to
>>> "chkp_get_invalid_op_bounds"
>>> and thus eventually pass this down as a flag to chkp_find_bounds (or check
>>> on call_arg again).
>>
>> Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to
>>
>> (gdb) p debug_tree(call_arg)
>>  <ssa_name 0x7ffff69d5120
>>     type <pointer_type 0x7ffff69c9e70
>>         type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
>>             sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836> unit size <var_decl 0x7ffff69d33f0 D.1837>
>>             align 32 symtab 0 alias set -1 structural equality domain <integer_type 0x7ffff69c9dc8>
>>             pointer_to_this <pointer_type 0x7ffff69c9e70>>
>>         unsigned DI
>>         size <integer_cst 0x7ffff6876cd8 constant 64>
>>         unit size <integer_cst 0x7ffff6876cf0 constant 8>
>>         align 64 symtab 0 alias set -1 structural equality>
>>     visited
>>     def_stmt x.2_7 = x.1_20;
>>     version 7>
>>
>> where bounds result in:
>>   _18 = _6 * 4;
>>   x.1_20 = __builtin_alloca_with_align (_18, 32);
>>   __bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);
>>
>> which is consider fine. I don't understand how can be that broken on gimple as it's not exposed
>> by GIMPLE?
> 
> Not sure how this relates to my question.

Hi.

You mentioned that GIMPLE does not properly represent passing arguments by reference. And I'm asking
whether it can happen that pass_by_reference returns true (in tree-chkp) and eventually a call is not
going to be a pass by reference?

> 
>>>
>>> Note that here 'type' and call_arg may not match (asking for trouble).  Plus
>>> 'fntype' is computed bogously (should use gimple_call_fntype).
>>>
>>> It later does sth like
>>>
>>>   /* For direct calls fndecl is replaced with instrumented version.  */
>>>   if (fndecl)
>>>     {
>>>       tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
>>>       gimple_call_set_fndecl (new_call, new_decl);
>>>       /* In case of a type cast we should modify used function
>>>          type instead of using type of new fndecl.  */
>>>       if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
>>>         {
>>>           tree type = gimple_call_fntype (call);
>>>           type = chkp_copy_function_type_adding_bounds (type);
>>>           gimple_call_set_fntype (new_call, type);
>>>         }
>>>       else
>>>         gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));
>>>
>>> but obviously if gimple_call_fntype and fndecl don't match cloning
>>> the mismatched decl isn't of very much help ... (it may just generate
>>> more mismatches with mismatching bounds...).
>>
>> That second issue should be probably fixed by comparing each formal and actual arguments,
>> where for a mismatch an invalid bounds should be returned? Am I right?
> 
> It's hard to say what to do for calls through incompatible tyes.  I'd
> say it should use
> the not instrumented copy (which there may be none if the function was
> static and we
> didn't need a clone?).  After all we're likely passing garbage to the
> function anyway.

Agree with you, as I briefly read the pass, there's quite fancy how it adds the shadow arguments
(bounds) to calls. What about always cloning fndecl and adding bounds just for arguments that
really correspond to gimple call fntype? Others can be preserved. It's definitely next stage1 material
and I'm wondering whether it worth for the afford?

P.S. I had a chat with Martin J. and it looks we have multiple places in source code where we have to somehow
deal with discrepancy between formal and actual arguments. He also mentioned that you tried to fix these issues
some time ago? But it's probably a hard mission?

> 
> Richard.
> 
>> Martin
>>
>>>
>>>> ...
>>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Martin
>>>>
>>

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Richard Biener]

On Mon, Mar 13, 2017 at 2:02 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/13/2017 01:28 PM, Richard Biener wrote:
>> On Thu, Mar 9, 2017 at 12:40 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 03/08/2017 12:00 PM, Richard Biener wrote:
>>>> On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>> On 03/07/2017 03:53 PM, Richard Biener wrote:
>>>>>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>>>>>> marxin <mliska@suse.cz> writes:
>>>>>>>>
>>>>>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>>>> new file mode 100644
>>>>>>>>> index 00000000000..f9223db1b2d
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>>>>>> @@ -0,0 +1,4 @@
>>>>>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>>>>>
>>>>>>>> ... and again: make this x86-only.
>>>>>>>>
>>>>>>>>       Rainer
>>>>>>>>
>>>>>>>
>>>>>>> Thanks. I'm sending v2 of the patch.
>>>>>>
>>>>>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>>>>>
>>>>>> Why not, instead of internal_error in the default: case do
>>>>>>
>>>>>>   bounds = chkp_get_invalid_op_bounds ();
>>>>>
>>>>> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
>>>>> security extension, I would be strict here in order to not handle something that can bypass
>>>>> the checking.
>>>>>
>>>>>>
>>>>>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>>>>>> a REAL_CST for example?
>>>>>
>>>>> It's called when setting bounds in a call expr:
>>>>>
>>>>> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
>>>>> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
>>>>> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
>>>>> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
>>>>> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528
>>>
>>> Yes, as you pointed out, we've got 2 issues here:
>>>
>>>>
>>>> So this happens for calls with mismatched arguments?  I believe that
>>>>
>>>>       if (BOUNDED_TYPE_P (type)
>>>>           || pass_by_reference (NULL, TYPE_MODE (type), type, true))
>>>>         new_args.safe_push (chkp_find_bounds (call_arg, gsi));
>>>>
>>>> may be misguided given pass_by_reference is not exposed at GIMPLE and thus
>>>> for the pass_by_reference case it should fall back to
>>>> "chkp_get_invalid_op_bounds"
>>>> and thus eventually pass this down as a flag to chkp_find_bounds (or check
>>>> on call_arg again).
>>>
>>> Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to
>>>
>>> (gdb) p debug_tree(call_arg)
>>>  <ssa_name 0x7ffff69d5120
>>>     type <pointer_type 0x7ffff69c9e70
>>>         type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
>>>             sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836> unit size <var_decl 0x7ffff69d33f0 D.1837>
>>>             align 32 symtab 0 alias set -1 structural equality domain <integer_type 0x7ffff69c9dc8>
>>>             pointer_to_this <pointer_type 0x7ffff69c9e70>>
>>>         unsigned DI
>>>         size <integer_cst 0x7ffff6876cd8 constant 64>
>>>         unit size <integer_cst 0x7ffff6876cf0 constant 8>
>>>         align 64 symtab 0 alias set -1 structural equality>
>>>     visited
>>>     def_stmt x.2_7 = x.1_20;
>>>     version 7>
>>>
>>> where bounds result in:
>>>   _18 = _6 * 4;
>>>   x.1_20 = __builtin_alloca_with_align (_18, 32);
>>>   __bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);
>>>
>>> which is consider fine. I don't understand how can be that broken on gimple as it's not exposed
>>> by GIMPLE?
>>
>> Not sure how this relates to my question.
>
> Hi.
>
> You mentioned that GIMPLE does not properly represent passing arguments by reference. And I'm asking
> whether it can happen that pass_by_reference returns true (in tree-chkp) and eventually a call is not
> going to be a pass by reference?

No, that can't happen.  I said that for example for

struct S { ... } s;
foo (s);

pass_by_reference may be true but on gimple you see a struct s as
actual argument.  I'm not sure
what chkp_find_bounds does to 's' in this case.  Like if floats are
passed by reference you might
see a REAL_CST passed to chkp_find_bounds but in reality it will get a
pointer (with bounds
according to the argument slot that was used).

>>
>>>>
>>>> Note that here 'type' and call_arg may not match (asking for trouble).  Plus
>>>> 'fntype' is computed bogously (should use gimple_call_fntype).
>>>>
>>>> It later does sth like
>>>>
>>>>   /* For direct calls fndecl is replaced with instrumented version.  */
>>>>   if (fndecl)
>>>>     {
>>>>       tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
>>>>       gimple_call_set_fndecl (new_call, new_decl);
>>>>       /* In case of a type cast we should modify used function
>>>>          type instead of using type of new fndecl.  */
>>>>       if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
>>>>         {
>>>>           tree type = gimple_call_fntype (call);
>>>>           type = chkp_copy_function_type_adding_bounds (type);
>>>>           gimple_call_set_fntype (new_call, type);
>>>>         }
>>>>       else
>>>>         gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));
>>>>
>>>> but obviously if gimple_call_fntype and fndecl don't match cloning
>>>> the mismatched decl isn't of very much help ... (it may just generate
>>>> more mismatches with mismatching bounds...).
>>>
>>> That second issue should be probably fixed by comparing each formal and actual arguments,
>>> where for a mismatch an invalid bounds should be returned? Am I right?
>>
>> It's hard to say what to do for calls through incompatible tyes.  I'd
>> say it should use
>> the not instrumented copy (which there may be none if the function was
>> static and we
>> didn't need a clone?).  After all we're likely passing garbage to the
>> function anyway.
>
> Agree with you, as I briefly read the pass, there's quite fancy how it adds the shadow arguments
> (bounds) to calls. What about always cloning fndecl and adding bounds just for arguments that
> really correspond to gimple call fntype? Others can be preserved. It's definitely next stage1 material
> and I'm wondering whether it worth for the afford?
>
> P.S. I had a chat with Martin J. and it looks we have multiple places in source code where we have to somehow
> deal with discrepancy between formal and actual arguments. He also mentioned that you tried to fix these issues
> some time ago? But it's probably a hard mission?

It's a hard mission indeed ;)

Richard.

>>
>> Richard.
>>
>>> Martin
>>>
>>>>
>>>>> ...
>>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> Martin
>>>>>
>>>
>

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

On 03/13/2017 02:07 PM, Richard Biener wrote:
> No, that can't happen.  I said that for example for
> 
> struct S { ... } s;
> foo (s);
> 
> pass_by_reference may be true but on gimple you see a struct s as
> actual argument.  I'm not sure
> what chkp_find_bounds does to 's' in this case.  Like if floats are
> passed by reference you might
> see a REAL_CST passed to chkp_find_bounds but in reality it will get a
> pointer (with bounds
> according to the argument slot that was used).

Currently constants have invalid bounds assigned. Thus I guess it can't cause a trouble
when such constant is used as a pointer. Anyhow, the discrepancy should be handled better.

Last question related to the patch. May I at least install the part which adds {COMPLEX,VECTOR}_CSTs
handling as it can happen with -mabi=ms (where formal and actual args do match)? Or using -mabi=ms + CHKP
does not make sense at all?

Martin

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Ilya Enkovich]

2017-03-13 16:33 GMT+03:00 Martin Liška <mliska@suse.cz>:
> On 03/13/2017 02:07 PM, Richard Biener wrote:
>> No, that can't happen.  I said that for example for
>>
>> struct S { ... } s;
>> foo (s);
>>
>> pass_by_reference may be true but on gimple you see a struct s as
>> actual argument.  I'm not sure
>> what chkp_find_bounds does to 's' in this case.  Like if floats are
>> passed by reference you might
>> see a REAL_CST passed to chkp_find_bounds but in reality it will get a
>> pointer (with bounds
>> according to the argument slot that was used).
>
> Currently constants have invalid bounds assigned. Thus I guess it can't cause a trouble
> when such constant is used as a pointer. Anyhow, the discrepancy should be handled better.
>
> Last question related to the patch. May I at least install the part which adds {COMPLEX,VECTOR}_CSTs
> handling as it can happen with -mabi=ms (where formal and actual args do match)? Or using -mabi=ms + CHKP
> does not make sense at all?

Hi,

Originally casted function calls were not taken into account by
instrumentation pass and it
caused such sloppy handling of these casts later. It would be nice to
revise this code.

Meanwhile your fix should go to trunk. CHKP instrumentation pass is
supposed to be target
independent and therefore shouldn't rely on any ABI.

Thanks,
Ilya

>
> Martin

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Martin Liška]

On 03/15/2017 12:58 AM, Ilya Enkovich wrote:
> 2017-03-13 16:33 GMT+03:00 Martin Liška <mliska@suse.cz>:
>> On 03/13/2017 02:07 PM, Richard Biener wrote:
>>> No, that can't happen.  I said that for example for
>>>
>>> struct S { ... } s;
>>> foo (s);
>>>
>>> pass_by_reference may be true but on gimple you see a struct s as
>>> actual argument.  I'm not sure
>>> what chkp_find_bounds does to 's' in this case.  Like if floats are
>>> passed by reference you might
>>> see a REAL_CST passed to chkp_find_bounds but in reality it will get a
>>> pointer (with bounds
>>> according to the argument slot that was used).
>>
>> Currently constants have invalid bounds assigned. Thus I guess it can't cause a trouble
>> when such constant is used as a pointer. Anyhow, the discrepancy should be handled better.
>>
>> Last question related to the patch. May I at least install the part which adds {COMPLEX,VECTOR}_CSTs
>> handling as it can happen with -mabi=ms (where formal and actual args do match)? Or using -mabi=ms + CHKP
>> does not make sense at all?
>
> Hi,
>
> Originally casted function calls were not taken into account by
> instrumentation pass and it
> caused such sloppy handling of these casts later. It would be nice to
> revise this code.

Hi.

Yep, code revisiting would be needed in this area.

>
> Meanwhile your fix should go to trunk. CHKP instrumentation pass is
> supposed to be target
> independent and therefore shouldn't rely on any ABI.

As you are MPX maintainer, may I consider that as patch approval?

Martin

>
> Thanks,
> Ilya
>
>>
>> Martin

Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

[Jeff Law]

On 03/15/2017 04:12 AM, Martin Liška wrote:
> On 03/15/2017 12:58 AM, Ilya Enkovich wrote:
>> 2017-03-13 16:33 GMT+03:00 Martin Liška <mliska@suse.cz>:
>>> On 03/13/2017 02:07 PM, Richard Biener wrote:
>>>> No, that can't happen.  I said that for example for
>>>>
>>>> struct S { ... } s;
>>>> foo (s);
>>>>
>>>> pass_by_reference may be true but on gimple you see a struct s as
>>>> actual argument.  I'm not sure
>>>> what chkp_find_bounds does to 's' in this case.  Like if floats are
>>>> passed by reference you might
>>>> see a REAL_CST passed to chkp_find_bounds but in reality it will get a
>>>> pointer (with bounds
>>>> according to the argument slot that was used).
>>>
>>> Currently constants have invalid bounds assigned. Thus I guess it
>>> can't cause a trouble
>>> when such constant is used as a pointer. Anyhow, the discrepancy
>>> should be handled better.
>>>
>>> Last question related to the patch. May I at least install the part
>>> which adds {COMPLEX,VECTOR}_CSTs
>>> handling as it can happen with -mabi=ms (where formal and actual args
>>> do match)? Or using -mabi=ms + CHKP
>>> does not make sense at all?
>>
>> Hi,
>>
>> Originally casted function calls were not taken into account by
>> instrumentation pass and it
>> caused such sloppy handling of these casts later. It would be nice to
>> revise this code.
>
> Hi.
>
> Yep, code revisiting would be needed in this area.
>
>>
>> Meanwhile your fix should go to trunk. CHKP instrumentation pass is
>> supposed to be target
>> independent and therefore shouldn't rely on any ABI.
>
> As you are MPX maintainer, may I consider that as patch approval?
You can consider it as approval.

jeff

Re: [PATCH] MPX: Fix option handling.

[Rainer Orth]

Jakub Jelinek <jakub@redhat.com> writes:

> On Fri, Mar 10, 2017 at 02:09:20PM +0100, Martin Liška wrote:
>> Hello.
>> 
>> This is follow-up patch which I agreed on with Jakub.
>> It enables CHKP with LSAN and majority of UBSAN options.
>> 
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>> 
>> Ready to be installed?
>> Martin
>
>> >From a410d5e4e028d34dc00b4aa637cdcd3986b971d8 Mon Sep 17 00:00:00 2001
>> From: marxin <mliska@suse.cz>
>> Date: Fri, 10 Mar 2017 11:05:27 +0100
>> Subject: [PATCH] MPX: Fix option handling.
>> 
>> gcc/ChangeLog:
>> 
>> 2017-03-10  Martin Liska  <mliska@suse.cz>
>> 
>>         PR target/65705
>>         PR target/69804
>> 	* toplev.c (process_options): Enable MPX with LSAN and UBSAN
>> 	(except -fsanitize=bounds and -fsanitize=bounds-strict).
>
> Please avoid the ()s, that is confusing with ()s used to surround
> function etc. names.  Just use UBSAN,
> 	except ... strict.
>
>> 	* tree-chkp.c (chkp_walk_pointer_assignments): Verify that
>> 	FIELD != NULL.
>
>> +	  error_at (UNKNOWN_LOCATION,
>> +		    "-fcheck-pointer-bounds is not supported with "
>> +		    "-fsanitize=bounds-strict");
>> +	  flag_check_pointer_bounds = 0;
>
> Given the recent i18n discussions, perhaps this ought to be
> %<-fcheck-pointer-bounds%> and %<-fsanitize=bounds-strict%> and similarly
> elsewhere (of course not for Address/Thread Sanitizer words).
>
> Ok for trunk either way.

Unfortunately, that last change broke gcc.target/i386/pr65044.c:

FAIL: gcc.target/i386/pr65044.c  (test for errors, line )
FAIL: gcc.target/i386/pr65044.c (test for excess errors)

seen e.g. on Linux/x86_64 and Solaris/x86.

We have

Excess errors:
cc1: error: '-fcheck-pointer-bounds' is not supported with Address Sanitizer

while the test expects

/* { dg-error "-fcheck-pointer-bounds is not supported with Address Sanitizer" "" { target *-*-* } 0 } */

I guess that, given gcc-dg.exp hardcodes LANG=C, changing the dg-error
to just include the single quotes should be enough.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] MPX: Fix option handling.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

On 03/17/2017 01:17 PM, Rainer Orth wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> 
>> On Fri, Mar 10, 2017 at 02:09:20PM +0100, Martin Liška wrote:
>>> Hello.
>>>
>>> This is follow-up patch which I agreed on with Jakub.
>>> It enables CHKP with LSAN and majority of UBSAN options.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>> Martin
>>
>>> >From a410d5e4e028d34dc00b4aa637cdcd3986b971d8 Mon Sep 17 00:00:00 2001
>>> From: marxin <mliska@suse.cz>
>>> Date: Fri, 10 Mar 2017 11:05:27 +0100
>>> Subject: [PATCH] MPX: Fix option handling.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2017-03-10  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR target/65705
>>>         PR target/69804
>>> 	* toplev.c (process_options): Enable MPX with LSAN and UBSAN
>>> 	(except -fsanitize=bounds and -fsanitize=bounds-strict).
>>
>> Please avoid the ()s, that is confusing with ()s used to surround
>> function etc. names.  Just use UBSAN,
>> 	except ... strict.
>>
>>> 	* tree-chkp.c (chkp_walk_pointer_assignments): Verify that
>>> 	FIELD != NULL.
>>
>>> +	  error_at (UNKNOWN_LOCATION,
>>> +		    "-fcheck-pointer-bounds is not supported with "
>>> +		    "-fsanitize=bounds-strict");
>>> +	  flag_check_pointer_bounds = 0;
>>
>> Given the recent i18n discussions, perhaps this ought to be
>> %<-fcheck-pointer-bounds%> and %<-fsanitize=bounds-strict%> and similarly
>> elsewhere (of course not for Address/Thread Sanitizer words).
>>
>> Ok for trunk either way.
> 
> Unfortunately, that last change broke gcc.target/i386/pr65044.c:
> 
> FAIL: gcc.target/i386/pr65044.c  (test for errors, line )
> FAIL: gcc.target/i386/pr65044.c (test for excess errors)
> 
> seen e.g. on Linux/x86_64 and Solaris/x86.
> 
> We have
> 
> Excess errors:
> cc1: error: '-fcheck-pointer-bounds' is not supported with Address Sanitizer
> 
> while the test expects
> 
> /* { dg-error "-fcheck-pointer-bounds is not supported with Address Sanitizer" "" { target *-*-* } 0 } */
> 
> I guess that, given gcc-dg.exp hardcodes LANG=C, changing the dg-error
> to just include the single quotes should be enough.
> 
> 	Rainer
> 

Will be fixed with following patch.
Similar to what we do in gcc/testsuite/objc.dg/fobjc-exceptions-1.m.

Martin

[-- Attachment #2: 0001-Fix-dg-error-for-a-test.patch --]
[-- Type: text/x-patch, Size: 1029 bytes --]

From cb4916381ee03cfb6c147a61e85ab1b6c7c634e9 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 21 Mar 2017 12:19:49 +0100
Subject: [PATCH] Fix dg-error for a test

gcc/testsuite/ChangeLog:

2017-03-21  Martin Liska  <mliska@suse.cz>

	* gcc.target/i386/pr65044.c: Add '.' in order to catch
	apostrophes.
---
 gcc/testsuite/gcc.target/i386/pr65044.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.target/i386/pr65044.c b/gcc/testsuite/gcc.target/i386/pr65044.c
index 9b0636339e5..d5cfecd15a9 100644
--- a/gcc/testsuite/gcc.target/i386/pr65044.c
+++ b/gcc/testsuite/gcc.target/i386/pr65044.c
@@ -1,6 +1,6 @@
 /* { dg-do compile { target { ! x32 } } } */
 /* { dg-options "-fcheck-pointer-bounds -mmpx -fsanitize=address" } */
-/* { dg-error "-fcheck-pointer-bounds is not supported with Address Sanitizer" "" { target *-*-* } 0 } */
+/* { dg-error ".-fcheck-pointer-bounds. is not supported with Address Sanitizer" "" { target *-*-* } 0 } */
 
 extern int x[];
 
-- 
2.12.0