[llvm-gcc] segfault on the following code

[llvmbot]

Bugzilla Link	200
Resolution	FIXED
Resolved on	Feb 22, 2010 12:47
Version	1.1
OS	All
Attachments	Reproduces the bug., Proposed fix to bug
Reporter	LLVM Bugzilla Contributor
CC	@lattner

Extended Description

llvm-gcc segfaults on the attached pre-processed code:

I've been chasing this one awhile, and it occurs because of a write to invalid
memory. The write, in turn, occurs because we are trying to initialize the
first element of an array that we think has zero length. However, I haven't
figured out why llvm-gcc thinks the array's length is zero.

I'm assigning this to myself as I'm tracking it and making progress.

[lattner]

This is a patch for llvm-gcc (against the Release 1.1 version) that seems to
fix the problem. It unfortunetly includes an (incorrect) work-around code for
PR#199.

Can you include an assertion or something that will keep #571 a compile-fail
instead of a miscompilation bug?

1. It hits an assert if the GCC tree does not specify which element an array
   initilizer is initializing. I don't believe the original code handled this
   case correctly.

It would be very good to write a testcase that triggers this code to find out.
I suspect that it either is not possible to trigger this because of funny GCC
stuff (if the code really was broken), or the code was correct. It is very
important to know which before checking in this patch.

2. It will resize an LLVM array type if a specified initializer value falls
   outside of the array's upper limit. For example, if the array has a current
   size of 5 and there is an initializer for the seventh element, the code
   expands the array to be 7 elements. This fixes the actual memory
   corruption bug.

You should use xrealloc instead of hand coding it. Also, generally, please use
better variable names than 'silly' :)

It might be good to add code so that we don't resize an array for which the
size was known ahead of time.

There are many optimizations that could be done in the C frontend. It is
complicated and twisted enough without them though, that I would prefer not to
start adding optimizations if they make things any more complex.

Since the code isn't folded into Chris's tree yet (and I'd like to get his
comments on it), I'm not marking the bug RESOLVED yet.

Please change the code to use xrealloc, and address the #​1 issue above, then
I'll 'check it in'.

-Chris

[lattner]

Just marking bugs that should be fixed for 1.2

[lattner]

Unless someone looks at this, it's not gunna be a 1.2 bug.

[llvmbot]

This fix has actually been cleaned up and checked in, but the code to reproduce
the bug will fail in an assertion before it hits the code that I fixed.

Should I leave this bug open or should I close it?

[lattner]

Ok, this bug is confusing. Please make sure that your "fix" for this bug didn't
turn 199 into a miscompilation from a compile-fail. If it did, we should revert
your fix, otherwise this bug should be closed. Hopefully after CGO I will get
to spend some quality time with the CFE and fix, once and for all, the type and
constant layout code.

Additionally, please file a new bug with a reduced testcase from this bug so
that we can get the "second" problem fixed.

Thanks John,

-Chris

[llvmbot]

I'll attempt to remove the confusion:

The attached code to reproduce this bug (PR#200) used to reproduce the problem
when my proposed fix for PR#199 was applied to llvm-gcc.

However, I never committed the proposed fix for PR#199 because it miscompiled
programs. Instead, I added an assertion that causes gcc to abort. This will
stay there until a proper fix is available.

The llvm-gcc code pertaining to this bug was still incorrect and could have
caused other programs to crash llvm-gcc. I commited a cleaned-up version of the
fix I proposed here (gcc-3.4/gcc/llvm-expand.c, revision 1.2) and tested it
before adding the assertion for PR#199.

The problem, then, is that I don't have a way to reproduce this bug in our
official llvm-gcc sources.

Since I'm pretty sure PR#200 is fixed, I'll close this bug as there's nothing
more to be done for it.