Patchwork Avoid uninitialized padding bits in REAL_CST/CONST_DOUBLE (PR middle-end/47903)

login
register
mail settings
Submitter Jakub Jelinek
Date Feb. 27, 2011, 6:16 p.m.
Message ID <20110227181619.GE30899@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/84699/
State New
Headers show

Comments

Jakub Jelinek - Feb. 27, 2011, 6:16 p.m.
On Sun, Feb 27, 2011 at 03:51:47PM +0100, Richard Guenther wrote:
> On Sun, Feb 27, 2011 at 1:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > In some places CONST_DOUBLE bits are compared as the HWIs, thus it is
> > undesirable to have uninitialized bits among them.  At least on LP64 hosts
> > there are 32 bits of padding between uexp and sign fields.
> > Most of the places in gcc that work with REAL_VALUE_TYPE seem to actually
> > either clear the whole REAL_VALUE_TYPE by memset, or start by copying
> > to result one of the arguments.  The exception seems to be real_arithmetic
> > for PLUS/MINUS/MULT/RDIV, where the result might be the same as either
> > of the operands, or might be completely different.  Either real_arithmetic
> > in those 4 cases could compare result ptr with both operands and if it is
> > not equal to either of them, clear it, or the following patch adjusts
> > the callers where they do it.
> 
> Hm, it seems fishy to compare CONST_DOUBLEs as HWIs if they have
> padding.  But well - I guess it was always that way.

In loc_cmp case it wants stable comparison for sorting, and real comparison
isn't suitable for that, as it e.g. considers +0. and -0. to be equal.
But the code in real_value_from_int_cst is quite old:
 31038     kenner   /* Clear all bits of the real value type so that we can later do
 31038     kenner      bitwise comparisons to see if two values are the same.  */
 69587      ghazi   memset (&d, 0, sizeof d);
so I think it loc_cmp isn't the only one which does something like that.

> That said, I still would prefer the clearing to be done in
> real_arithmetic.  Just patching the existing callers won't save us
> from introducing more cases like this.

Here it is, bootstrapped/regtested on x86_64-linux and i686-linux,
verified with valgrind on the testcase:

2011-02-27  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/47903
	* real.c (real_arithmetic) <case PLUS_EXPR, MINUS_EXPR,
	MULT_EXPR, RDIV_EXPR>: Clear padding bits in *r first if
	r isn't op0 nor op1.



	Jakub
Richard Guenther - Feb. 27, 2011, 7:29 p.m.
On Sun, Feb 27, 2011 at 7:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Feb 27, 2011 at 03:51:47PM +0100, Richard Guenther wrote:
>> On Sun, Feb 27, 2011 at 1:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > In some places CONST_DOUBLE bits are compared as the HWIs, thus it is
>> > undesirable to have uninitialized bits among them.  At least on LP64 hosts
>> > there are 32 bits of padding between uexp and sign fields.
>> > Most of the places in gcc that work with REAL_VALUE_TYPE seem to actually
>> > either clear the whole REAL_VALUE_TYPE by memset, or start by copying
>> > to result one of the arguments.  The exception seems to be real_arithmetic
>> > for PLUS/MINUS/MULT/RDIV, where the result might be the same as either
>> > of the operands, or might be completely different.  Either real_arithmetic
>> > in those 4 cases could compare result ptr with both operands and if it is
>> > not equal to either of them, clear it, or the following patch adjusts
>> > the callers where they do it.
>>
>> Hm, it seems fishy to compare CONST_DOUBLEs as HWIs if they have
>> padding.  But well - I guess it was always that way.
>
> In loc_cmp case it wants stable comparison for sorting, and real comparison
> isn't suitable for that, as it e.g. considers +0. and -0. to be equal.
> But the code in real_value_from_int_cst is quite old:
>  31038     kenner   /* Clear all bits of the real value type so that we can later do
>  31038     kenner      bitwise comparisons to see if two values are the same.  */
>  69587      ghazi   memset (&d, 0, sizeof d);
> so I think it loc_cmp isn't the only one which does something like that.
>
>> That said, I still would prefer the clearing to be done in
>> real_arithmetic.  Just patching the existing callers won't save us
>> from introducing more cases like this.
>
> Here it is, bootstrapped/regtested on x86_64-linux and i686-linux,
> verified with valgrind on the testcase:

Ok.

Thanks,
Richard.

> 2011-02-27  Jakub Jelinek  <jakub@redhat.com>
>
>        PR middle-end/47903
>        * real.c (real_arithmetic) <case PLUS_EXPR, MINUS_EXPR,
>        MULT_EXPR, RDIV_EXPR>: Clear padding bits in *r first if
>        r isn't op0 nor op1.
>
> --- gcc/real.c.jj       2010-12-02 13:15:24.000000000 +0100
> +++ gcc/real.c  2011-02-27 16:55:43.474134359 +0100
> @@ -1009,15 +1009,25 @@ real_arithmetic (REAL_VALUE_TYPE *r, int
>   switch (code)
>     {
>     case PLUS_EXPR:
> +      /* Clear any padding areas in *r if it isn't equal to one of the
> +        operands so that we can later do bitwise comparisons later on.  */
> +      if (r != op0 && r != op1)
> +       memset (r, '\0', sizeof (*r));
>       return do_add (r, op0, op1, 0);
>
>     case MINUS_EXPR:
> +      if (r != op0 && r != op1)
> +       memset (r, '\0', sizeof (*r));
>       return do_add (r, op0, op1, 1);
>
>     case MULT_EXPR:
> +      if (r != op0 && r != op1)
> +       memset (r, '\0', sizeof (*r));
>       return do_multiply (r, op0, op1);
>
>     case RDIV_EXPR:
> +      if (r != op0 && r != op1)
> +       memset (r, '\0', sizeof (*r));
>       return do_divide (r, op0, op1);
>
>     case MIN_EXPR:
>
>
>        Jakub
>

Patch

--- gcc/real.c.jj	2010-12-02 13:15:24.000000000 +0100
+++ gcc/real.c	2011-02-27 16:55:43.474134359 +0100
@@ -1009,15 +1009,25 @@  real_arithmetic (REAL_VALUE_TYPE *r, int
   switch (code)
     {
     case PLUS_EXPR:
+      /* Clear any padding areas in *r if it isn't equal to one of the
+	 operands so that we can later do bitwise comparisons later on.  */
+      if (r != op0 && r != op1)
+	memset (r, '\0', sizeof (*r));
       return do_add (r, op0, op1, 0);
 
     case MINUS_EXPR:
+      if (r != op0 && r != op1)
+	memset (r, '\0', sizeof (*r));
       return do_add (r, op0, op1, 1);
 
     case MULT_EXPR:
+      if (r != op0 && r != op1)
+	memset (r, '\0', sizeof (*r));
       return do_multiply (r, op0, op1);
 
     case RDIV_EXPR:
+      if (r != op0 && r != op1)
+	memset (r, '\0', sizeof (*r));
       return do_divide (r, op0, op1);
 
     case MIN_EXPR: