[08/08] PR/64003 workaround (uninit memory in i386.md)

I suspect this is papering over a real problem, but I've been
applying this workaround locally to keep my valgrind output clean.

gcc/ChangeLog:
	PR/64003
	* final.c (shorten_branches): Allocate insn_lengths with
	XCNEWVEC rather than XNEWVEC; remove subsequent per-element
	initialization.
---
 gcc/final.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

On 11/25/14 18:39, David Malcolm wrote:
> I suspect this is papering over a real problem, but I've been
> applying this workaround locally to keep my valgrind output clean.
>
> gcc/ChangeLog:
> 	PR/64003
> 	* final.c (shorten_branches): Allocate insn_lengths with
> 	XCNEWVEC rather than XNEWVEC; remove subsequent per-element
> 	initialization.
I'd like more information on this one.

My first thought is that something must be creating a new insn after 
shorten_branches is complete or an existing insn that was not on the 
chain when we called shorten-branches, but got threaded onto the chain 
later.  Either would be considered bad in various ways, so we'd like to 
fix it.

Presumably you're not doing an out-of-range access into insn_lengths? 
Right?

Do you have a reasonable testcase for this?

jeff

On Wed, 2014-11-26 at 10:41 -0700, Jeff Law wrote:
> On 11/25/14 18:39, David Malcolm wrote:
> > I suspect this is papering over a real problem, but I've been
> > applying this workaround locally to keep my valgrind output clean.
> >
> > gcc/ChangeLog:
> > 	PR/64003
> > 	* final.c (shorten_branches): Allocate insn_lengths with
> > 	XCNEWVEC rather than XNEWVEC; remove subsequent per-element
> > 	initialization.
> I'd like more information on this one.

I've spent some time trying to track this down, and I've added detailed
notes to the bug.

In short, I believe the problem occurs with a "*jcc_1" insn that jumps
forwards, but the full details are in the bug.

> My first thought is that something must be creating a new insn after 
> shorten_branches is complete or an existing insn that was not on the 
> chain when we called shorten-branches, but got threaded onto the chain 
> later.  Either would be considered bad in various ways, so we'd like to 
> fix it.

I don't think either of these are the case.  I believe it's due to the
size of the jcc_1 insn being affected by the distance to the jump
target, which for a forward jump is a bit of a chicken-and-egg issue,
since that distance is affected by the size of the jcc_1 insn itself.  

It looks like align_fuzz exists in order to cope with this kind of
circular definition, but the issue seems to occur inside align_fuzz
itself.

> Presumably you're not doing an out-of-range access into insn_lengths? 
> Right?
> 
> Do you have a reasonable testcase for this?

I've added a more minimal reproducer to the bug (8 lines); see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64003#c7

I see it with latest trunk (r218254) on x86_64.

See my notes in the bug.

Dave

On 12/02/14 09:20, David Malcolm wrote:
> In short, I believe the problem occurs with a "*jcc_1" insn that jumps
> forwards, but the full details are in the bug.
>
>> My first thought is that something must be creating a new insn after
>> shorten_branches is complete or an existing insn that was not on the
>> chain when we called shorten-branches, but got threaded onto the chain
>> later.  Either would be considered bad in various ways, so we'd like to
>> fix it.
>
> I don't think either of these are the case.  I believe it's due to the
> size of the jcc_1 insn being affected by the distance to the jump
> target, which for a forward jump is a bit of a chicken-and-egg issue,
> since that distance is affected by the size of the jcc_1 insn itself.
>
> It looks like align_fuzz exists in order to cope with this kind of
> circular definition, but the issue seems to occur inside align_fuzz
> itself.
Sorry, I didn't look at the BZ, you had already put a fair amount of 
analysis in there.  My bad.

This feels sooooo familiar.

Jeff

On 12/02/14 09:20, David Malcolm wrote:
>
> I've spent some time trying to track this down, and I've added detailed
> notes to the bug.
>
> In short, I believe the problem occurs with a "*jcc_1" insn that jumps
> forwards, but the full details are in the bug.
>
>> My first thought is that something must be creating a new insn after
>> shorten_branches is complete or an existing insn that was not on the
>> chain when we called shorten-branches, but got threaded onto the chain
>> later.  Either would be considered bad in various ways, so we'd like to
>> fix it.
>
> I don't think either of these are the case.  I believe it's due to the
> size of the jcc_1 insn being affected by the distance to the jump
> target, which for a forward jump is a bit of a chicken-and-egg issue,
> since that distance is affected by the size of the jcc_1 insn itself.
I thought we had code somewhere that made worst case assumptions about 
the lengths, to get those arrays initialized with reasonable value 
without looking forward.  But that may have also been in place prior to 
the addition of align_fuzz.

>
> It looks like align_fuzz exists in order to cope with this kind of
> circular definition, but the issue seems to occur inside align_fuzz
> itself.
align_fuzz is meant to estimate padding due to alignments.  Joern and I 
went round and round on that code and I never managed to wrap my head 
around it or its correctness.  Jim W. might have stepped in at some 
point to arbitrate.  I may have to pull out my archives and review that 
history (IIRC is predates egcs).

Interestingly there was a great discussion around the code in 2009 which 
mirrored some of my initial concerns with this code.  Ultimately Bernd 
recommended removing it, but nobody reviewed/approved the patch.

I think you should ping Joern to chime in here about the proper course 
of action to take if we're going to keep this code in GCC.  You could 
just assign in the BZ to him.  FWIW, I can't convince myself 
initializing the value to zero is actually safe or not.


jeff