diff mbox series

RFA: Fix uninitialized memory use in sched_macro_fuse_insns

Message ID CAMqJFCrS0AKhWTEYiGUdkNV4kJOgQh=wEfb1-8O7b0CQQLP02Q@mail.gmail.com
State New
Headers show
Series RFA: Fix uninitialized memory use in sched_macro_fuse_insns | expand

Commit Message

Joern Rennecke April 4, 2019, 11:26 p.m. UTC 
sched_macro_fuse_insns uses the value in condreg1 without
checking the return value of targetm.fixed_condition_code_regs.  As
this variables
is not initialized anywhere, this leads to constructing cc_reg_1 with
an undefined value,
and then using that in reg_referenced_p, if TARGET_FIXED_CONDITION_CODE_REGS
has the default value as defined in target.def (hook_bool_uintp_uintp_false).

The attached patch fixes this by checking the return value of
targetm.fixed_condition_code_regs.  Bootstrapped & regtested on
x86_64-pc-linux-gnu .
2019-04-04  Joern Rennecke  <joern.rennecke@embecosm.com>

	* sched-deps.c (sched_macro_fuse_insns): Check return value of
	targetm.fixed_condition_code_regs.

Comments

Richard Sandiford April 5, 2019, 10:07 a.m. UTC | #1 
Joern Rennecke <joern.rennecke@embecosm.com> writes:
> sched_macro_fuse_insns uses the value in condreg1 without
> checking the return value of targetm.fixed_condition_code_regs.  As
> this variables
> is not initialized anywhere, this leads to constructing cc_reg_1 with
> an undefined value,
> and then using that in reg_referenced_p, if TARGET_FIXED_CONDITION_CODE_REGS
> has the default value as defined in target.def (hook_bool_uintp_uintp_false).
>
> The attached patch fixes this by checking the return value of
> targetm.fixed_condition_code_regs.  Bootstrapped & regtested on
> x86_64-pc-linux-gnu .
>
> 2019-04-04  Joern Rennecke  <joern.rennecke@embecosm.com>
>
> 	* sched-deps.c (sched_macro_fuse_insns): Check return value of
> 	targetm.fixed_condition_code_regs.

OK, thanks.

Richard
Joern Rennecke April 5, 2019, 2:34 p.m. UTC | #2 
On Fri, 5 Apr 2019 at 11:07, Richard Sandiford
<richard.sandiford@arm.com> wrote:


> > 2019-04-04  Joern Rennecke  <joern.rennecke@embecosm.com>
> >
> >       * sched-deps.c (sched_macro_fuse_insns): Check return value of
> >       targetm.fixed_condition_code_regs.
>
> OK, thanks.

Thanks for the review.

Is that OK restricted to delayed applying once the gcc 9 branch has
been cut and gcc 10 stage 1 opened (because the bug is not a
regression unless going back to 2013)
or also OK to apply to the current 9.0.0 trunk (since this should be a
safe patch and leaving the bug in might hinder debugging to find
actual regressions) ?
Richard Sandiford April 5, 2019, 2:47 p.m. UTC | #3 
Joern Rennecke <joern.rennecke@embecosm.com> writes:
> On Fri, 5 Apr 2019 at 11:07, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>
>
>> > 2019-04-04  Joern Rennecke  <joern.rennecke@embecosm.com>
>> >
>> >       * sched-deps.c (sched_macro_fuse_insns): Check return value of
>> >       targetm.fixed_condition_code_regs.
>>
>> OK, thanks.
>
> Thanks for the review.
>
> Is that OK restricted to delayed applying once the gcc 9 branch has
> been cut and gcc 10 stage 1 opened (because the bug is not a
> regression unless going back to 2013)
> or also OK to apply to the current 9.0.0 trunk (since this should be a
> safe patch and leaving the bug in might hinder debugging to find
> actual regressions) ?

OK now IMO.  A regression from 2013 is still a regression, and like
you say, it should be very safe.  I think arm is the only affected
in-tree port, and it's only going to help there.

Thanks,
Richard
Jeff Law April 9, 2019, 3:08 p.m. UTC | #4 
On 4/5/19 8:47 AM, Richard Sandiford wrote:
> Joern Rennecke <joern.rennecke@embecosm.com> writes:
>> On Fri, 5 Apr 2019 at 11:07, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>
>>
>>>> 2019-04-04  Joern Rennecke  <joern.rennecke@embecosm.com>
>>>>
>>>>       * sched-deps.c (sched_macro_fuse_insns): Check return value of
>>>>       targetm.fixed_condition_code_regs.
>>>
>>> OK, thanks.
>>
>> Thanks for the review.
>>
>> Is that OK restricted to delayed applying once the gcc 9 branch has
>> been cut and gcc 10 stage 1 opened (because the bug is not a
>> regression unless going back to 2013)
>> or also OK to apply to the current 9.0.0 trunk (since this should be a
>> safe patch and leaving the bug in might hinder debugging to find
>> actual regressions) ?
> 
> OK now IMO.  A regression from 2013 is still a regression, and like
> you say, it should be very safe.  I think arm is the only affected
> in-tree port, and it's only going to help there.
Agreed.  Even if we didn't have an active regression, this kind of error
is painful enough to debug that I'd rather have it squashed now :-)

jeff
diff mbox series

Patch

Index: sched-deps.c
===================================================================
--- sched-deps.c	(revision 270146)
+++ sched-deps.c	(working copy)
@@ -2857,14 +2857,16 @@  sched_macro_fuse_insns (rtx_insn *insn)
     {
       unsigned int condreg1, condreg2;
       rtx cc_reg_1;
-      targetm.fixed_condition_code_regs (&condreg1, &condreg2);
-      cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
-      if (reg_referenced_p (cc_reg_1, PATTERN (insn))
-	  && modified_in_p (cc_reg_1, prev))
+      if (targetm.fixed_condition_code_regs (&condreg1, &condreg2))
 	{
-	  if (targetm.sched.macro_fusion_pair_p (prev, insn))
-	    SCHED_GROUP_P (insn) = 1;
-	  return;
+	  cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
+	  if (reg_referenced_p (cc_reg_1, PATTERN (insn))
+	      && modified_in_p (cc_reg_1, prev))
+	    {
+	      if (targetm.sched.macro_fusion_pair_p (prev, insn))
+		SCHED_GROUP_P (insn) = 1;
+	      return;
+	    }
 	}
     }