diff mbox

Fix reload1.c warning for some targets

Message ID 87lhcn6gn4.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Sept. 3, 2015, 8:39 a.m. UTC 
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
> 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

Sorry for the slow reply.  I did try to coerce the current uninit
pass to handle this case, but the patch I ended up with only worked
by accident because of the strange way in which the pass handles
limit cases.  (If we have more than MAX_NUM_CHAINS chains, it silently
drops the excess chains and continues regardless, so it's quite easy
to come up with cases where the predicates for either the definition
or the use consider an arbitrary subset of the actual conditions.)

It sounds like Jeff has a much more radical rewrite in mind,
so for now how about just turning -Wmaybe-uninitialized into
a warning for this function?  The patch will mean that it becomes
a warning even if someone turns off warnings on the command line,
but I don't think that's important.

Bootstrapped and regression-tested on x86_64-linux-gnu.  Also tested
with a cross-compiler to sparc-linux-gnu (which also triggered the
warning for me).  Tested that clang could still compile the file.
OK to install?


gcc/
	* reload1.c (elimination_costs_in_insn): Locally turn
	-Wmaybe-uninitialized into a warning.

Comments

Jeff Law Sept. 4, 2015, 8:04 p.m. UTC | #1 
On 09/03/2015 02:39 AM, Richard Sandiford wrote:
>
> It sounds like Jeff has a much more radical rewrite in mind,
Certainly not anything on the immediate horizon.  The amount of block 
copying that'd be needed to isolate the jump threading path would be 
significant.

I do wonder if we should be looking at a way to mark paths which have 
jump threading opportunities, but which we do not optimize and exploit 
that information in the uninit and other analysis.

Bodik had a paper on those concepts as well.  He was mostly looking at 
how to account for those paths in code coverage analysis, but there may 
be something useful in there.

> so for now how about just turning -Wmaybe-uninitialized into
> a warning for this function?  The patch will mean that it becomes
> a warning even if someone turns off warnings on the command line,
> but I don't think that's important.
>
> Bootstrapped and regression-tested on x86_64-linux-gnu.  Also tested
> with a cross-compiler to sparc-linux-gnu (which also triggered the
> warning for me).  Tested that clang could still compile the file.
> OK to install?
>
>
> gcc/
> 	* reload1.c (elimination_costs_in_insn): Locally turn
> 	-Wmaybe-uninitialized into a warning.
I can live with this.  Though I'd appreciate it if someone could reduce 
the sparcv9 testcase and create a bug to track it too.

jeff
Richard Sandiford Sept. 10, 2015, 7:19 p.m. UTC | #2 
Jeff Law <law@redhat.com> writes:
> On 09/03/2015 02:39 AM, Richard Sandiford wrote:
>> so for now how about just turning -Wmaybe-uninitialized into
>> a warning for this function?  The patch will mean that it becomes
>> a warning even if someone turns off warnings on the command line,
>> but I don't think that's important.
>>
>> Bootstrapped and regression-tested on x86_64-linux-gnu.  Also tested
>> with a cross-compiler to sparc-linux-gnu (which also triggered the
>> warning for me).  Tested that clang could still compile the file.
>> OK to install?
>>
>>
>> gcc/
>> 	* reload1.c (elimination_costs_in_insn): Locally turn
>> 	-Wmaybe-uninitialized into a warning.
> I can live with this.  Though I'd appreciate it if someone could reduce 
> the sparcv9 testcase and create a bug to track it too.

Applied, thanks.

Richard
diff mbox

Patch

diff --git a/gcc/reload1.c b/gcc/reload1.c
index ad243e3..c7cc37b 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -3636,6 +3636,8 @@  eliminate_regs_in_insn (rtx_insn *insn, int replace)
    eliminations in its operands and record cases where eliminating a reg with
    an invariant equivalence would add extra cost.  */
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic warning "-Wmaybe-uninitialized"
 static void
 elimination_costs_in_insn (rtx_insn *insn)
 {
@@ -3785,6 +3787,7 @@  elimination_costs_in_insn (rtx_insn *insn)
 
   return;
 }
+#pragma GCC diagnostic pop
 
 /* Loop through all elimination pairs.
    Recalculate the number not at initial offset.