Ping.
On Tue, Sep 17, 2013 at 12:32:03PM +0200, Marek Polacek wrote:
> 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
Marek
