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 <pola...@redhat.com>
Jakub Jelinek <ja...@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
