Patchwork Fix PR56990

login
register
mail settings
Submitter Marek Polacek
Date April 22, 2013, 4:53 p.m.
Message ID <20130422165359.GI13346@redhat.com>
Download mbox | patch
Permalink /patch/238609/
State New
Headers show

Comments

Marek Polacek - April 22, 2013, 4:53 p.m.
On Mon, Apr 22, 2013 at 05:23:17PM +0100, Richard Henderson wrote:
> On 2013-04-22 17:21, Jakub Jelinek wrote:
> >On Mon, Apr 22, 2013 at 06:11:26PM +0200, Marek Polacek wrote:
> >>We're getting SIGFPE, because one simply does not divide by zero.
> >>Fixed by doing the modulo only when size != 0.
> >>
> >>Regtested/bootstrapped on x86_64-linux, ok for trunk and 4.8?
> >>
> >>2013-04-22  Marek Polacek  <polacek@redhat.com>
> >>
> >>	PR sanitizer/56990
> >>	* tsan.c (instrument_expr): Don't count modulo if the size
> >>	is zero.
> >>
> >>	* gcc.dg/pr56990.c: New test.
> >>
> >>--- gcc/tsan.c.mp	2013-04-19 15:39:46.416450528 +0200
> >>+++ gcc/tsan.c	2013-04-22 17:23:54.115647673 +0200
> >>@@ -131,7 +131,8 @@ instrument_expr (gimple_stmt_iterator gs
> >>    if (TREE_READONLY (base))
> >>      return false;
> >>
> >>-  if (bitpos % (size * BITS_PER_UNIT)
> >>+  if ((size != 0
> >
> >IMHO for size == 0 we should return false too, there is no store or read to
> >be instrumented.  So
> >     if (size == 0
> >	|| bitpos % (size * BITS_PER_UNIT)
> >	|| bitsize != size * BITS_PER_UNIT)
> >       return false;
> >?
> 
> Point.  And along those lines, will we ever have bitsize == 0?
> If not, then just swapping the conditions is sufficient:

Well, actually the bitsize is 0 in this case, so only swapping the
conditions wouldn't suffice.  So this is what Jakub suggested and
also my first version of the patch ;).

Ok?

2013-04-22  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/56990
	* tsan.c (instrument_expr): Don't instrument expression
	in case its size is zero.

	* gcc.dg/pr56990.c: New test.



m~
Jakub Jelinek - April 22, 2013, 4:54 p.m.
On Mon, Apr 22, 2013 at 06:53:59PM +0200, Marek Polacek wrote:
> 2013-04-22  Marek Polacek  <polacek@redhat.com>
> 
> 	PR sanitizer/56990
> 	* tsan.c (instrument_expr): Don't instrument expression
> 	in case its size is zero.
> 
> 	* gcc.dg/pr56990.c: New test.

Yes, thanks.

> --- gcc/tsan.c.mp	2013-04-19 15:39:46.416450528 +0200
> +++ gcc/tsan.c	2013-04-22 18:49:57.834052631 +0200
> @@ -131,7 +131,8 @@ instrument_expr (gimple_stmt_iterator gs
>    if (TREE_READONLY (base))
>      return false;
>  
> -  if (bitpos % (size * BITS_PER_UNIT)
> +  if (size == 0
> +      || bitpos % (size * BITS_PER_UNIT)
>        || bitsize != size * BITS_PER_UNIT)
>      return false;
>  
> --- gcc/testsuite/gcc.dg/pr56990.c.mp	2013-04-22 17:30:14.523876683 +0200
> +++ gcc/testsuite/gcc.dg/pr56990.c	2013-04-22 17:29:06.704666252 +0200
> @@ -0,0 +1,10 @@
> +/* PR sanitizer/56990 */
> +/* { dg-do compile { target { x86_64-*-linux* && lp64 } } } */
> +/* { dg-options "-fsanitize=thread" } */
> +
> +struct S{};
> +
> +void foo(struct S *p)
> +{
> +  *p = (struct S){};
> +}
> 
> 
> m~

	Jakub

Patch

--- gcc/tsan.c.mp	2013-04-19 15:39:46.416450528 +0200
+++ gcc/tsan.c	2013-04-22 18:49:57.834052631 +0200
@@ -131,7 +131,8 @@  instrument_expr (gimple_stmt_iterator gs
   if (TREE_READONLY (base))
     return false;
 
-  if (bitpos % (size * BITS_PER_UNIT)
+  if (size == 0
+      || bitpos % (size * BITS_PER_UNIT)
       || bitsize != size * BITS_PER_UNIT)
     return false;
 
--- gcc/testsuite/gcc.dg/pr56990.c.mp	2013-04-22 17:30:14.523876683 +0200
+++ gcc/testsuite/gcc.dg/pr56990.c	2013-04-22 17:29:06.704666252 +0200
@@ -0,0 +1,10 @@ 
+/* PR sanitizer/56990 */
+/* { dg-do compile { target { x86_64-*-linux* && lp64 } } } */
+/* { dg-options "-fsanitize=thread" } */
+
+struct S{};
+
+void foo(struct S *p)
+{
+  *p = (struct S){};
+}