| Message ID | 20130917103203.GG23899@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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 Marek
On Wed, Sep 25, 2013 at 10:34:42AM +0200, Marek Polacek wrote: > > 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. Ok. Jakub
--- 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; +}