Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns
On 4/5/19 8:47 AM, Richard Sandiford wrote: > Joern Rennecke writes: >> On Fri, 5 Apr 2019 at 11:07, Richard Sandiford >> wrote: >> >> 2019-04-04 Joern Rennecke * 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
Re: RFA: Fix uninitialized memory use in sched_macro_fuse_insns
Joern Rennecke writes: > On Fri, 5 Apr 2019 at 11:07, Richard Sandiford > wrote: > > >> > 2019-04-04 Joern Rennecke >> > >> > * 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
On Fri, 5 Apr 2019 at 11:07, Richard Sandiford wrote: > > 2019-04-04 Joern Rennecke > > > > * 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
Joern Rennecke 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 > > * sched-deps.c (sched_macro_fuse_insns): Check return value of > targetm.fixed_condition_code_regs. OK, thanks. Richard
RFA: Fix uninitialized memory use in sched_macro_fuse_insns
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 * 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 (, ); - 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 (, )) { - 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; + } } }