Patchwork fix newly-introduced buglet in tree-ssa-sccvn.c

login
register
mail settings
Submitter Nathan Froyd
Date Oct. 9, 2010, 1:10 a.m.
Message ID <20101009011006.GT17388@nightcrawler>
Download mbox | patch
Permalink /patch/67311/
State New
Headers show

Comments

Nathan Froyd - Oct. 9, 2010, 1:10 a.m.
My recent cleanup of tree-ssa-sccvn.c added a bug along the way.
Prior to my patch, inserting a VN by pieces did:

  if (length >= 1)
    vno1->op[0] = op0;
  if (length >= 2)
    vno1->op[1] = op1;
  if (length >= 3)
    vno1->op[2] = op2;
  if (length >= 4)
    vno1->op[3] = op3;

and looking up a VN by pieces did:

  vno1.op[0] = op0;
  vno1.op[1] = op1;
  vno1.op[2] = op2;
  vno1.op[3] = op3;

and I unified these by using the second snippet of code.

As you might have guessed, this change is not correct.  When we're
inserting a VN by pieces, we allocate a vn_nary_opt structure for a
specific number of operands.  If we use the second form, then we
scribble over memory beyond the end of our allocation, causing havoc.
(The specific case Julian noticed was overwriting libc's data structures
for obstacks when doing a cross to arm-none-linux-gnueabi and compiling
glibc...I am surprised this memory corruption was not discovered
earlier.)

The patch below fixes this by using a variant of the first snippet
above.  I think this is correct even for looking up a VN, since we'll
only ever iterate over vno->length operands when computing the hash for
the VN.

Bootstrapped on x86_64-unknown-linux-gnu and verifying that the patch
fixes compiling glibc for arm-none-linux-gnueabi.  OK to commit?

-Nathan

	* tree-ssa-sccvn.c (init_vn_nary_op_from_pieces): Consult length
	before initializing vno->op.

Index: ChangeLog
===================================================================
Andrew Pinski - Oct. 9, 2010, 1:33 a.m.
On Fri, Oct 8, 2010 at 6:10 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> My recent cleanup of tree-ssa-sccvn.c added a bug along the way.
> Prior to my patch, inserting a VN by pieces did:

I think this is also PR 45950 which is also seen on i686-linux-gnu so
maybe checking there too.


Thanks,
Andrew Pinski
H.J. Lu - Oct. 9, 2010, 5:29 a.m.
On Fri, Oct 8, 2010 at 6:10 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> My recent cleanup of tree-ssa-sccvn.c added a bug along the way.
> Prior to my patch, inserting a VN by pieces did:
>
>  if (length >= 1)
>    vno1->op[0] = op0;
>  if (length >= 2)
>    vno1->op[1] = op1;
>  if (length >= 3)
>    vno1->op[2] = op2;
>  if (length >= 4)
>    vno1->op[3] = op3;
>
> and looking up a VN by pieces did:
>
>  vno1.op[0] = op0;
>  vno1.op[1] = op1;
>  vno1.op[2] = op2;
>  vno1.op[3] = op3;
>
> and I unified these by using the second snippet of code.
>
> As you might have guessed, this change is not correct.  When we're
> inserting a VN by pieces, we allocate a vn_nary_opt structure for a
> specific number of operands.  If we use the second form, then we
> scribble over memory beyond the end of our allocation, causing havoc.
> (The specific case Julian noticed was overwriting libc's data structures
> for obstacks when doing a cross to arm-none-linux-gnueabi and compiling
> glibc...I am surprised this memory corruption was not discovered
> earlier.)
>
>
> The patch below fixes this by using a variant of the first snippet
> above.  I think this is correct even for looking up a VN, since we'll
> only ever iterate over vno->length operands when computing the hash for
> the VN.
>
> Bootstrapped on x86_64-unknown-linux-gnu and verifying that the patch
> fixes compiling glibc for arm-none-linux-gnueabi.  OK to commit?
>
> -Nathan
>
>        * tree-ssa-sccvn.c (init_vn_nary_op_from_pieces): Consult length
>        before initializing vno->op.

It also fixed:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45952

May I suggest you also bootstrap i686-linux on Linux/x86-64? My example
in PR 45952 works on Fedora/x86-64.

Thanks.


H.J.
Eric Botcazou - Oct. 9, 2010, 8:56 a.m.
> Index: tree-ssa-sccvn.c
> ===================================================================
> --- tree-ssa-sccvn.c	(revision 165212)
> +++ tree-ssa-sccvn.c	(working copy)
> @@ -1708,10 +1708,15 @@ init_vn_nary_op_from_pieces (vn_nary_op_
>    vno->opcode = code;
>    vno->length = length;
>    vno->type = type;
> -  vno->op[0] = op0;
> -  vno->op[1] = op1;
> -  vno->op[2] = op2;
> -  vno->op[3] = op3;
> +  switch (length)
> +    {
> +    case 4: vno->op[3] = op3;
> +    case 3: vno->op[2] = op2;
> +    case 2: vno->op[1] = op1;
> +    case 1: vno->op[0] = op0;
> +    default:
> +      break;
> +    }

Add a comment stating that all the fallthrus are intended.  Otherwise this is 
obviously correct so please install to fix the breakage.
Richard Guenther - Oct. 9, 2010, 10:54 a.m.
On Sat, Oct 9, 2010 at 10:56 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Index: tree-ssa-sccvn.c
>> ===================================================================
>> --- tree-ssa-sccvn.c  (revision 165212)
>> +++ tree-ssa-sccvn.c  (working copy)
>> @@ -1708,10 +1708,15 @@ init_vn_nary_op_from_pieces (vn_nary_op_
>>    vno->opcode = code;
>>    vno->length = length;
>>    vno->type = type;
>> -  vno->op[0] = op0;
>> -  vno->op[1] = op1;
>> -  vno->op[2] = op2;
>> -  vno->op[3] = op3;
>> +  switch (length)
>> +    {
>> +    case 4: vno->op[3] = op3;
>> +    case 3: vno->op[2] = op2;
>> +    case 2: vno->op[1] = op1;
>> +    case 1: vno->op[0] = op0;
>> +    default:
>> +      break;
>> +    }
>
> Add a comment stating that all the fallthrus are intended.  Otherwise this is
> obviously correct so please install to fix the breakage.

The change is ok.

Richard.

> --
> Eric Botcazou
>

Patch

Index: tree-ssa-sccvn.c
===================================================================
--- tree-ssa-sccvn.c	(revision 165212)
+++ tree-ssa-sccvn.c	(working copy)
@@ -1708,10 +1708,15 @@  init_vn_nary_op_from_pieces (vn_nary_op_
   vno->opcode = code;
   vno->length = length;
   vno->type = type;
-  vno->op[0] = op0;
-  vno->op[1] = op1;
-  vno->op[2] = op2;
-  vno->op[3] = op3;
+  switch (length)
+    {
+    case 4: vno->op[3] = op3;
+    case 3: vno->op[2] = op2;
+    case 2: vno->op[1] = op1;
+    case 1: vno->op[0] = op0;
+    default:
+      break;
+    }
 }
 
 /* Initialize VNO from OP.  */