RFA: Fix uninitialized memory use in sched_macro_fuse_insns

RFA: Fix uninitialized memory use in sched_macro_fuse_insns

[Joern Rennecke]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

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 .

[-- Attachment #2: no-fixed-cond-patch.txt --]
[-- Type: text/plain, Size: 1105 bytes --]

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.

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;
+	    }
 	}
     }
 

Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns

[Richard Sandiford]

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

Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns

[Joern Rennecke]

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) ?

Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns

[Richard Sandiford]

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

Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns

[Jeff Law]

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