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)


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


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