This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Don't always instrument shifts (PR sanitizer/58413)


On Mon, Sep 16, 2013 at 08:35:35PM +0200, Jakub Jelinek wrote:
> On Fri, Sep 13, 2013 at 08:01:36PM +0200, Marek Polacek wrote:
> I'd say the above is going to be a maintainance nightmare, with all the code
> duplication.  And you are certainly going to miss cases that way,
> e.g.
> void
> foo (void)
> {
>   int A[-2 / -1] = {};
> }
> 
> I'd say instead of adding all this, you should just at the right spot insert
> if (integer_zerop (t)) return NULL_TREE; or similar.
> 
> For shift instrumentation, I guess you could add
> if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
>   return NULL_TREE;
> right before:
>   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);

Yeah, this is _much_ better.  I'm glad we can live without that
ugliness.

> > +/* PR sanitizer/58413 */
> > +/* { dg-do run } */
> > +/* { dg-options "-fsanitize=shift -w" } */
> > +
> > +int x = 7;
> > +int
> > +main (void)
> > +{
> > +  /* All of the following should pass.  */
> > +  int A[128 >> 5] = {};
> > +  int B[128 << 5] = {};
> > +
> > +  static int e =
> > +    ((int)
> > +     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> > +      ((0) << ((15) + 6)) | ((0) << (15))));
> 
> This relies on int32plus, so needs to be /* { dg-do run { target int32plus } } */

Fixed.

> > --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3	2013-09-13 18:25:06.195847144 +0200
> > +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-13 19:16:38.990211229 +0200
> > @@ -0,0 +1,21 @@
> > +/* { dg-do compile} */
> > +/* { dg-options "-fsanitize=shift -w" } */
> > +/* { dg-shouldfail "ubsan" } */
> > +
> > +int x;
> > +int
> > +main (void)
> > +{
> > +  /* None of the following should pass.  */
> > +  switch (x)
> > +    {
> > +    case 1 >> -1:	/* { dg-error "" } */
> > +    case -1 >> -1:	/* { dg-error "" } */
> > +    case 1 << -1:	/* { dg-error "" } */
> > +    case -1 << -1:	/* { dg-error "" } */
> > +    case -1 >> 200:	/* { dg-error "" } */
> > +    case 1 << 200:	/* { dg-error "" } */
> 
> Can't you fill in the error you are expecting, or is the problem
> that the wording is very different between C and C++?

I discovered { target c } stuff, so I filled in both error messages.

This patch seems to work: bootstrap-ubsan passes + ubsan testsuite
passes too.  Ok for trunk?

2013-09-17  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/58413
c-family/
	* c-ubsan.c (ubsan_instrument_shift): Don't instrument
	an expression if we can prove it is correct.
	(ubsan_instrument_division): Likewise.  Remove unnecessary
	check.

testsuite/
	* c-c++-common/ubsan/shift-4.c: New test.
	* c-c++-common/ubsan/shift-5.c: New test.
	* c-c++-common/ubsan/div-by-zero-5.c: New test.
	* gcc.dg/ubsan/c-shift-1.c: New test.

--- gcc/c-family/c-ubsan.c.mp	2013-09-17 12:24:44.582835840 +0200
+++ gcc/c-family/c-ubsan.c	2013-09-17 12:24:48.772849823 +0200
@@ -51,14 +51,6 @@ ubsan_instrument_division (location_t lo
   if (TREE_CODE (type) != INTEGER_TYPE)
     return NULL_TREE;
 
-  /* If we *know* that the divisor is not -1 or 0, we don't have to
-     instrument this expression.
-     ??? We could use decl_constant_value to cover up more cases.  */
-  if (TREE_CODE (op1) == INTEGER_CST
-      && integer_nonzerop (op1)
-      && !integer_minus_onep (op1))
-    return NULL_TREE;
-
   t = fold_build2 (EQ_EXPR, boolean_type_node,
 		    op1, build_int_cst (type, 0));
 
@@ -74,6 +66,11 @@ ubsan_instrument_division (location_t lo
       t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
     }
 
+  /* If the condition was folded to 0, no need to instrument
+     this expression.  */
+  if (integer_zerop (t))
+    return NULL_TREE;
+
   /* In case we have a SAVE_EXPR in a conditional context, we need to
      make sure it gets evaluated before the condition.  */
   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
@@ -138,6 +135,11 @@ ubsan_instrument_shift (location_t loc,
       tt = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, x, tt);
     }
 
+  /* If the condition was folded to 0, no need to instrument
+     this expression.  */
+  if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
+    return NULL_TREE;
+
   /* In case we have a SAVE_EXPR in a conditional context, we need to
      make sure it gets evaluated before the condition.  */
   t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
--- gcc/testsuite/c-c++-common/ubsan/shift-4.c.mp	2013-09-17 12:25:12.130931875 +0200
+++ gcc/testsuite/c-c++-common/ubsan/shift-4.c	2013-09-17 10:19:44.665199565 +0200
@@ -0,0 +1,30 @@
+/* PR sanitizer/58413 */
+/* { dg-do run { target int32plus } } */
+/* { dg-options "-fsanitize=shift -w" } */
+
+int x = 7;
+int
+main (void)
+{
+  /* All of the following should pass.  */
+  int A[128 >> 5] = {};
+  int B[128 << 5] = {};
+
+  static int e =
+    ((int)
+     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
+      ((0) << ((15) + 6)) | ((0) << (15))));
+
+  if (e != 503316480)
+    __builtin_abort ();
+
+  switch (x)
+    {
+    case 1 >> 4:
+    case 1 << 4:
+    case 128 << (4 + 1):
+    case 128 >> (4 + 1):
+      return 1;
+    }
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp	2013-09-17 12:25:17.549952906 +0200
+++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-17 12:22:47.658413113 +0200
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -w" } */
+/* { dg-shouldfail "ubsan" } */
+
+int x;
+int
+foo (void)
+{
+  /* None of the following should pass.  */
+  switch (x)
+    {
+    case 1 >> -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 12 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 12 } */
+    case -1 >> -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 15 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 15 } */
+    case 1 << -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 18 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 18 } */
+    case -1 << -1:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 21 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 21 } */
+    case -1 >> 200:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 24 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 24 } */
+    case 1 << 200:
+/* { dg-error "case label does not reduce to an integer constant" "" {target c } 27 } */
+/* { dg-error "is not a constant expression" "" { target c++ } 27 } */
+      return 1;
+    }
+  return 0;
+}
--- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c.mp	2013-09-17 12:25:05.910907983 +0200
+++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c	2013-09-17 10:19:44.724199779 +0200
@@ -0,0 +1,8 @@
+/* { dg-do compile} */
+/* { dg-options "-fsanitize=integer-divide-by-zero" } */
+
+void
+foo (void)
+{
+  int A[-2 / -1] = {};
+}
--- gcc/testsuite/gcc.dg/ubsan/c-shift-1.c.mp	2013-09-17 12:25:23.533975060 +0200
+++ gcc/testsuite/gcc.dg/ubsan/c-shift-1.c	2013-09-17 10:19:44.725199783 +0200
@@ -0,0 +1,18 @@
+/* { dg-do compile} */
+/* { dg-options "-fsanitize=shift -w" } */
+/* { dg-shouldfail "ubsan" } */
+
+int x;
+int
+main (void)
+{
+  /* None of the following should pass.  */
+  int A[1 >> -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
+  int B[-1 >> -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+  int D[1 << -1] = {};    /* { dg-error "variable-sized object may not be initialized" } */
+  int E[-1 << -1] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+  int F[-1 >> 200] = {};  /* { dg-error "variable-sized object may not be initialized" } */
+  int G[1 << 200] = {};   /* { dg-error "variable-sized object may not be initialized" } */
+
+  return 0;
+}

	Marek


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]