Message ID | CAMqJFCrS0AKhWTEYiGUdkNV4kJOgQh=wEfb1-8O7b0CQQLP02Q@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | RFA: Fix uninitialized memory use in sched_macro_fuse_insns | expand |
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
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) ?
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
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
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; + } } }