| Message ID | 20101009011006.GT17388@nightcrawler |
|---|---|
| State | New |
| Headers | show |
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
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.
> 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.
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 >
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. */
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 ===================================================================