[backport from gcc-4.7/trunk r180284 ] From: Mike Stump Subject: regcprop.c bug fix Date: Wed, 19 Oct 2011 14:24:03 -0700 List-Archive: So while tracking down a hairy address reload for an output reload bug, copyprop_hardreg_forward_1 was faulting because it was trying to extract move patterns that didn't work out, and when it came back to the code, it then tries to access recog_data, but the problem is, the exploration of other instructions to see if they match, overwrites that data, and there is nothing that restores the data to a point in which the code below this point expects. It uses recog_data.operand[i], where i is limited by n_ops, but that value corresponded to the old data in recog_data. The recog and extract_insn in insn_invalid_p called from verify_changes called from apply_change_group called from validate_change wipes the `old' recog_data with new data. This data, for example, might only have 2 operands, with an invalid value for the third operand. The old n_ops, might well be 3 from the original data. Accessing that data can cause a crash. So, I don't know if people want to data regenerated, or it they want to cache the data, or if they want to save and restore it underneath a called api... Date: Thu, 20 Oct 2011 15:22:49 +0200 From: Bernd Schmidt Subject: Re: regcprop.c bug fix List-Archive: On 10/19/11 23:24, Mike Stump wrote: > So while tracking down a hairy address reload for an output reload > bug, copyprop_hardreg_forward_1 was faulting because it was trying to > extract move patterns that didn't work out, and when it came back to > the code, it then tries to access recog_data, but the problem is, the > exploration of other instructions to see if they match, overwrites > that data, and there is nothing that restores the data to a point in > which the code below this point expects. It uses > recog_data.operand[i], where i is limited by n_ops, but that value > corresponded to the old data in recog_data. The recog and > extract_insn in insn_invalid_p called from verify_changes called from > apply_change_group called from validate_change wipes the `old' > recog_data with new data. This data, for example, might only have 2 > operands, with an invalid value for the third operand. The old > n_ops, might well be 3 from the original data. Accessing that data > can cause a crash. I found that maximally confusing, so let me try to rephrase it to see if I understood you. The two calls to validate_change clobber the recog_data even if they fail. In case they failed, we want to continue looking at data from the original insn, so we must recompute it. If that's what you were trying to say, it looks like the right diagnosis. Better to move the recomputation into the if statement that contains the validate_change calls and possibly add a comment about the effect of that function; otherwise OK. Subject: Re: regcprop.c bug fix From: Mike Stump Date: Thu, 20 Oct 2011 19:16:41 -0700 List-Archive: On Oct 20, 2011, at 6:22 AM, Bernd Schmidt wrote: > I found that maximally confusing, so let me try to rephrase it to see if > I understood you. The two calls to validate_change clobber the > recog_data even if they fail. In case they failed, we want to continue > looking at data from the original insn, so we must recompute it. Yes, exactly. > diagnosis. Better to move the recomputation into the if statement that > contains the validate_change calls and possibly add a comment about the > effect of that function; otherwise OK. Ok, I've updated the code and added some comments and finished testing and = checked it in. Thanks. gcc/ 2011-10-20 Mike Stump * regcprop.c (copyprop_hardreg_forward_1): Update recog_data after validate_change wipes it out. --- gcc-4.6.2/gcc/regcprop.c.~1~ 2010-12-17 23:51:25.000000000 +0100 +++ gcc-4.6.2/gcc/regcprop.c 2011-12-26 23:26:46.000000000 +0100 @@ -841,6 +841,12 @@ copyprop_hardreg_forward_1 (basic_block changed = true; goto did_replacement; } + /* We need to re-extract as validate_change clobbers + recog_data. */ + extract_insn (insn); + if (! constrain_operands (1)) + fatal_insn_not_found (insn); + preprocess_constraints (); } /* Otherwise, try all valid registers and see if its valid. */ @@ -863,6 +869,12 @@ copyprop_hardreg_forward_1 (basic_block changed = true; goto did_replacement; } + /* We need to re-extract as validate_change clobbers + recog_data. */ + extract_insn (insn); + if (! constrain_operands (1)) + fatal_insn_not_found (insn); + preprocess_constraints (); } } }