| Message ID | 87d1z1kedx.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On 08/05/2015 08:18 AM, Richard Sandiford wrote: > Building some targets results in a warning about orig_dup[i] potentially > being used uninitialised. I think the warning is fair, since it isn't > obvious that the reog_data-based loop bound remains unchanged between: > > for (i = 0; i < recog_data.n_dups; i++) > orig_dup[i] = *recog_data.dup_loc[i]; > > and: > > for (i = 0; i < recog_data.n_dups; i++) > *recog_data.dup_loc[i] = orig_dup[i]; > > Tested on x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > gcc/ > * reload1.c (elimination_costs_in_insn): Make it obvious to the > compiler that the n_dups and n_operands loop bounds are invariant. There's a BZ about this issue. 55035. What I want is to make sure we don't lose track of the false positive in 55035 (caused by a miss jump thread due to aliasing issues). So perhaps the way forward is to install your change and twiddle the summary of 55035 in some way that makes it more obvious the bz tracks a false positive from -Wuninitialized and attach 55035 to the -Wuninitialized meta bug (24639). Does that work for you? Jeff
Jeff Law <law@redhat.com> writes: > On 08/05/2015 08:18 AM, Richard Sandiford wrote: >> Building some targets results in a warning about orig_dup[i] potentially >> being used uninitialised. I think the warning is fair, since it isn't >> obvious that the reog_data-based loop bound remains unchanged between: >> >> for (i = 0; i < recog_data.n_dups; i++) >> orig_dup[i] = *recog_data.dup_loc[i]; >> >> and: >> >> for (i = 0; i < recog_data.n_dups; i++) >> *recog_data.dup_loc[i] = orig_dup[i]; >> >> Tested on x86_64-linux-gnu. OK to install? >> >> Thanks, >> Richard >> >> gcc/ >> * reload1.c (elimination_costs_in_insn): Make it obvious to the >> compiler that the n_dups and n_operands loop bounds are invariant. > There's a BZ about this issue. 55035. > > What I want is to make sure we don't lose track of the false positive in > 55035 (caused by a miss jump thread due to aliasing issues). > > So perhaps the way forward is to install your change and twiddle the > summary of 55035 in some way that makes it more obvious the bz tracks a > false positive from -Wuninitialized and attach 55035 to the > -Wuninitialized meta bug (24639). Is it really a false positive in this case though? We have: for (i = 0; i < recog_data.n_dups; i++) orig_dup[i] = *recog_data.dup_loc[i]; for (i = 0; i < recog_data.n_operands; i++) { orig_operand[i] = recog_data.operand[i]; /* For an asm statement, every operand is eliminable. */ if (insn_is_asm || insn_data[icode].operand[i].eliminable) { bool is_set_src, in_plus; /* Check for setting a register that we know about. */ if (recog_data.operand_type[i] != OP_IN && REG_P (orig_operand[i])) { /* If we are assigning to a register that can be eliminated, it must be as part of a PARALLEL, since the code above handles single SETs. We must indicate that we can no longer eliminate this reg. */ for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++) if (ep->from_rtx == orig_operand[i]) ep->can_eliminate = 0; } /* Companion to the above plus substitution, we can allow invariants as the source of a plain move. */ is_set_src = false; if (old_set && recog_data.operand_loc[i] == &SET_SRC (old_set)) is_set_src = true; if (is_set_src && !sets_reg_p) note_reg_elim_costly (SET_SRC (old_set), insn); in_plus = false; if (plus_src && sets_reg_p && (recog_data.operand_loc[i] == &XEXP (plus_src, 0) || recog_data.operand_loc[i] == &XEXP (plus_src, 1))) in_plus = true; eliminate_regs_1 (recog_data.operand[i], VOIDmode, NULL_RTX, is_set_src || in_plus, true); /* Terminate the search in check_eliminable_occurrences at this point. */ *recog_data.operand_loc[i] = 0; } } for (i = 0; i < recog_data.n_dups; i++) *recog_data.dup_loc[i] = *recog_data.operand_loc[(int) recog_data.dup_num[i]]; /* If any eliminable remain, they aren't eliminable anymore. */ check_eliminable_occurrences (old_body); /* Restore the old body. */ for (i = 0; i < recog_data.n_operands; i++) *recog_data.operand_loc[i] = orig_operand[i]; for (i = 0; i < recog_data.n_dups; i++) *recog_data.dup_loc[i] = orig_dup[i]; and I don't see how GCC could prove that eliminate_regs_1 doesn't modify the value of recog_data.n_dups between the two loops. eliminate_regs_1 calls functions like plus_constant that are defined outside the TU and that certainly aren't pure/const. So I think c#5 (marked as a bogus reduction) is an accurate reflection of what reload1.c does. c#4 looks like a genuine bug but seems different from the reload1.c case. If we still warn for c#4 then I think we should keep the bugzilla entry open for that, but the warning for the reload1.c code seems justified. Perhaps the question is why it doesn't trigger on more targets :-) Thanks, Richard
On 08/05/2015 11:32 AM, Richard Sandiford wrote: > and I don't see how GCC could prove that eliminate_regs_1 doesn't > modify the value of recog_data.n_dups between the two loops. > eliminate_regs_1 calls functions like plus_constant that are defined > outside the TU and that certainly aren't pure/const. Right. I should have been clearer. I don't think the reload1.c code is a false positive because we can't see into those functions to determine side effects. > So I think c#5 (marked as a bogus reduction) is an accurate reflection > of what reload1.c does. c#4 looks like a genuine bug but seems different > from the reload1.c case. If we still warn for c#4 then I think we > should keep the bugzilla entry open for that, but the warning for the > reload1.c code seems justified. Right. I don't want to lose the false positive and associated missed jump threading in c#4. Perhaps the question is why it doesn't trigger on more targets :-) Not sure. Could be how match_dup is used plus some interactions with SRA and BRANCH_COST and who knows what else :-0 Jeff
On 08/05/2015 08:18 AM, Richard Sandiford wrote: > Building some targets results in a warning about orig_dup[i] potentially > being used uninitialised. I think the warning is fair, since it isn't > obvious that the reog_data-based loop bound remains unchanged between: > > for (i = 0; i < recog_data.n_dups; i++) > orig_dup[i] = *recog_data.dup_loc[i]; > > and: > > for (i = 0; i < recog_data.n_dups; i++) > *recog_data.dup_loc[i] = orig_dup[i]; > > Tested on x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > gcc/ > * reload1.c (elimination_costs_in_insn): Make it obvious to the > compiler that the n_dups and n_operands loop bounds are invariant. So thinking more about this, I think the best way forward is to: 1. Create a new BZ with the false positive extracted from c#4. 2. Install your patch and close 55035. I'll take care of #1, you can handle #2. jeff
Jeff Law <law@redhat.com> writes: > On 08/05/2015 08:18 AM, Richard Sandiford wrote: >> Building some targets results in a warning about orig_dup[i] potentially >> being used uninitialised. I think the warning is fair, since it isn't >> obvious that the reog_data-based loop bound remains unchanged between: >> >> for (i = 0; i < recog_data.n_dups; i++) >> orig_dup[i] = *recog_data.dup_loc[i]; >> >> and: >> >> for (i = 0; i < recog_data.n_dups; i++) >> *recog_data.dup_loc[i] = orig_dup[i]; >> >> Tested on x86_64-linux-gnu. OK to install? >> >> Thanks, >> Richard >> >> gcc/ >> * reload1.c (elimination_costs_in_insn): Make it obvious to the >> compiler that the n_dups and n_operands loop bounds are invariant. > So thinking more about this, I think the best way forward is to: > > 1. Create a new BZ with the false positive extracted from c#4. > > 2. Install your patch and close 55035. > > I'll take care of #1, you can handle #2. Thanks, I've now done #2. Richard
On 08/13/2015 02:29 PM, Richard Sandiford wrote: > Jeff Law <law@redhat.com> writes: >> On 08/05/2015 08:18 AM, Richard Sandiford wrote: >>> Building some targets results in a warning about orig_dup[i] potentially >>> being used uninitialised. I think the warning is fair, since it isn't >>> obvious that the reog_data-based loop bound remains unchanged between: >>> >>> for (i = 0; i < recog_data.n_dups; i++) >>> orig_dup[i] = *recog_data.dup_loc[i]; >>> >>> and: >>> >>> for (i = 0; i < recog_data.n_dups; i++) >>> *recog_data.dup_loc[i] = orig_dup[i]; >>> >>> Tested on x86_64-linux-gnu. OK to install? >>> >>> Thanks, >>> Richard >>> >>> gcc/ >>> * reload1.c (elimination_costs_in_insn): Make it obvious to the >>> compiler that the n_dups and n_operands loop bounds are invariant. >> So thinking more about this, I think the best way forward is to: >> >> 1. Create a new BZ with the false positive extracted from c#4. >> >> 2. Install your patch and close 55035. >> >> I'll take care of #1, you can handle #2. > > Thanks, I've now done #2. THanks. I've got the new BZ in place. So we both pop one item off our stacks. jeff
Richard Sandiford <rdsandiford@googlemail.com> writes: > Jeff Law <law@redhat.com> writes: >> On 08/05/2015 08:18 AM, Richard Sandiford wrote: >>> Building some targets results in a warning about orig_dup[i] potentially >>> being used uninitialised. I think the warning is fair, since it isn't >>> obvious that the reog_data-based loop bound remains unchanged between: >>> >>> for (i = 0; i < recog_data.n_dups; i++) >>> orig_dup[i] = *recog_data.dup_loc[i]; >>> >>> and: >>> >>> for (i = 0; i < recog_data.n_dups; i++) >>> *recog_data.dup_loc[i] = orig_dup[i]; >>> >>> Tested on x86_64-linux-gnu. OK to install? >>> >>> Thanks, >>> Richard >>> >>> gcc/ >>> * reload1.c (elimination_costs_in_insn): Make it obvious to the >>> compiler that the n_dups and n_operands loop bounds are invariant. >> So thinking more about this, I think the best way forward is to: >> >> 1. Create a new BZ with the false positive extracted from c#4. >> >> 2. Install your patch and close 55035. >> >> I'll take care of #1, you can handle #2. > > Thanks, I've now done #2. Unfortunately the patch broke sparcv9-sun-solaris2* (only, sparc-sun-solaris2* is fine) bootstrap: /vol/gcc/src/hg/trunk/local/gcc/reload1.c: In function 'void elimination_costs_in_insn(rtx_insn*)': /vol/gcc/src/hg/trunk/local/gcc/reload1.c:3772:41: error: 'orig_dup[1]' may be used uninitialized in this function [-Werror=maybe-uninitialized] *recog_data.dup_loc[i] = orig_dup[i]; ^ /vol/gcc/src/hg/trunk/local/gcc/reload1.c:3772:41: error: 'orig_dup[0]' may be used uninitialized in this function [-Werror=maybe-uninitialized] Rainer
diff --git a/gcc/reload1.c b/gcc/reload1.c index ce06e06..ad243e3 100644 --- a/gcc/reload1.c +++ b/gcc/reload1.c @@ -3708,10 +3708,12 @@ elimination_costs_in_insn (rtx_insn *insn) /* Eliminate all eliminable registers occurring in operands that can be handled by reload. */ extract_insn (insn); - for (i = 0; i < recog_data.n_dups; i++) + int n_dups = recog_data.n_dups; + for (i = 0; i < n_dups; i++) orig_dup[i] = *recog_data.dup_loc[i]; - for (i = 0; i < recog_data.n_operands; i++) + int n_operands = recog_data.n_operands; + for (i = 0; i < n_operands; i++) { orig_operand[i] = recog_data.operand[i]; @@ -3756,7 +3758,7 @@ elimination_costs_in_insn (rtx_insn *insn) } } - for (i = 0; i < recog_data.n_dups; i++) + for (i = 0; i < n_dups; i++) *recog_data.dup_loc[i] = *recog_data.operand_loc[(int) recog_data.dup_num[i]]; @@ -3764,9 +3766,9 @@ elimination_costs_in_insn (rtx_insn *insn) check_eliminable_occurrences (old_body); /* Restore the old body. */ - for (i = 0; i < recog_data.n_operands; i++) + for (i = 0; i < n_operands; i++) *recog_data.operand_loc[i] = orig_operand[i]; - for (i = 0; i < recog_data.n_dups; i++) + for (i = 0; i < n_dups; i++) *recog_data.dup_loc[i] = orig_dup[i]; /* Update all elimination pairs to reflect the status after the current
Building some targets results in a warning about orig_dup[i] potentially being used uninitialised. I think the warning is fair, since it isn't obvious that the reog_data-based loop bound remains unchanged between: for (i = 0; i < recog_data.n_dups; i++) orig_dup[i] = *recog_data.dup_loc[i]; and: for (i = 0; i < recog_data.n_dups; i++) *recog_data.dup_loc[i] = orig_dup[i]; Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * reload1.c (elimination_costs_in_insn): Make it obvious to the compiler that the n_dups and n_operands loop bounds are invariant.