diff mbox

Fix reload1.c warning for some targets

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

Commit Message

Richard Sandiford Aug. 5, 2015, 2:18 p.m. UTC
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.

Comments

Jeff Law Aug. 5, 2015, 5:01 p.m. UTC | #1
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
Richard Sandiford Aug. 5, 2015, 5:32 p.m. UTC | #2
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 < &reg_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
Jeff Law Aug. 11, 2015, 8:04 p.m. UTC | #3
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
Jeff Law Aug. 12, 2015, 5:16 p.m. UTC | #4
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
Richard Sandiford Aug. 13, 2015, 8:29 p.m. UTC | #5
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
Jeff Law Aug. 13, 2015, 8:33 p.m. UTC | #6
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
Rainer Orth Aug. 24, 2015, 10:50 a.m. UTC | #7
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 mbox

Patch

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