Patchwork regcprop.c bug fix

login
register
mail settings
Submitter Mike Stump
Date Oct. 19, 2011, 9:24 p.m.
Message ID <E7350094-FEF1-429A-853B-958DC6124557@comcast.net>
Download mbox | patch
Permalink /patch/120693/
State New
Headers show

Comments

Mike Stump - Oct. 19, 2011, 9:24 p.m.
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...

Ok?
Bernd Schmidt - Oct. 20, 2011, 1:22 p.m.
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.


Bernd

Patch

Index: regcprop.c
===================================================================
--- regcprop.c	(revision 1831)
+++ regcprop.c	(working copy)
@@ -866,6 +866,10 @@  copyprop_hardreg_forward_1 (basic_block
 		}
 	    }
 	}
+      extract_insn (insn);
+      if (! constrain_operands (1))
+	fatal_insn_not_found (insn);
+      preprocess_constraints ();
       no_move_special_case:
 
       any_replacements = false;