Patchwork Ouch: romcc "x[0] |= something" patch causes another crash

Stefan Reinauer <stepan@coresystems.de> writes:

> On 3/15/10 8:28 PM, ron minnich wrote:
>> On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>
>>   
>>> My practical concern is that there is no support for the general case where
>>> you do:
>>> char array[5];
>>> for (i = 0; i < 5; i++) {
>>>    array[i] = 7;
>>> }
>>>     
>> The bigger problem is that people are trying to take this compiler
>> beyond what makes sense (to me). I'm not sure we're going to cease to
>> exist if we don't have arrays.
>>   
> I agree we don't necessarily need to have such arrays. I think we just
> naturally assumed it should work,
> so no attempt to sneak anything in.
>
> What's implemented and what is not is hidden in Eric's brain and in a
> single file of 25k lines of code.
>
> If we don't want non-static non-const arrays, can we easily detect them
> in the code and give the user an error message that is better than a
> segfault?
> "You're a fool because you used non-const non-static arrays in romcc"
> would be fine. Just dropping dead without error is certainly less helpful.

We should be able to. I thought such a warning existed.  So I have been
scratching my head a bit to understand why it doesn't.

>> If romcc can't do something, then work around it; we can warn people
>> about no arrays. But given that nobody has the time to really support
>> romcc (as, e.g., gcc or llvm are supported) we're taking some real
>> risks just plugging changes in.
>>   
> The changes were merely trying to fix the segfaults, not implement or
> change anything big. I think we do want fixes for segfaults. Always.

Fixing segfaults and then getting code that doesn't work correctly
is worse.  Always.  I did my best with romcc to ensure the compile
fails if we are not going to generate the correct code.

At the same time I would much rather it be an assert than a random
segfault.

I have to run but I think this patch adds the missing check to
catch non-static arrays.


Eric

On 3/15/10 9:18 PM, Eric W. Biederman wrote:
>>> If romcc can't do something, then work around it; we can warn people
>>> about no arrays. But given that nobody has the time to really support
>>> romcc (as, e.g., gcc or llvm are supported) we're taking some real
>>> risks just plugging changes in.
>>>   
>>>       
>> The changes were merely trying to fix the segfaults, not implement or
>> change anything big. I think we do want fixes for segfaults. Always.
>>     
> Fixing segfaults and then getting code that doesn't work correctly
> is worse.  Always. 
Absolutely true. We're just only learning that we were this stupid :-)

>  I did my best with romcc to ensure the compile
> fails if we are not going to generate the correct code.
>
> At the same time I would much rather it be an assert than a random
> segfault.
>
> I have to run but I think this patch adds the missing check to
> catch non-static arrays.
>
>   
Seems to do the job... Please send a Signed-off-by: for the books:
http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

and this will make it into the tree.

You can of course check it in yourself if you wish, in any case this is
Acked-by: Stefan Reinauer <stepan@coresystems.de>

> Index: romcc.c
> ===================================================================
> --- romcc.c	(revision 4892)
> +++ romcc.c	(working copy)
> @@ -13458,6 +13458,10 @@
>  	if ((type->type & TYPE_MASK) == TYPE_FUNCTION) {
>  		error(state, 0, "Function prototypes not supported");
>  	}
> +	if (ident &&
> +		((type->type & TYPE_MASK) == TYPE_ARRAY) &&
> +		((type->type & STOR_MASK) != STOR_STATIC))
> +		error(state, 0, "non static arrays not supported");
>  	if (ident && 
>  		((type->type & STOR_MASK) == STOR_STATIC) &&
>  		((type->type & QUAL_CONST) == 0)) {
>
> Eric
>
>

Stefan Reinauer <stepan@coresystems.de> writes:

> Seems to do the job... Please send a Signed-off-by: for the books:
> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

> and this will make it into the tree.
>
> You can of course check it in yourself if you wish, in any case this is
> Acked-by: Stefan Reinauer <stepan@coresystems.de>

Just my bad habit of not signing off on patches I haven't even tested.


>> Index: romcc.c
>> ===================================================================
>> --- romcc.c	(revision 4892)
>> +++ romcc.c	(working copy)
>> @@ -13458,6 +13458,10 @@
>>  	if ((type->type & TYPE_MASK) == TYPE_FUNCTION) {
>>  		error(state, 0, "Function prototypes not supported");
>>  	}
>> +	if (ident &&
>> +		((type->type & TYPE_MASK) == TYPE_ARRAY) &&
>> +		((type->type & STOR_MASK) != STOR_STATIC))
>> +		error(state, 0, "non static arrays not supported");
>>  	if (ident && 
>>  		((type->type & STOR_MASK) == STOR_STATIC) &&
>>  		((type->type & QUAL_CONST) == 0)) {
>>

Eric

On 3/16/10 1:31 AM, Eric W. Biederman wrote:
> Stefan Reinauer <stepan@coresystems.de> writes:
>
>   
>> Seems to do the job... Please send a Signed-off-by: for the books:
>> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure
>>     
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>   
Thanks a lot.. r5230

>> and this will make it into the tree.
>>
>> You can of course check it in yourself if you wish, in any case this is
>> Acked-by: Stefan Reinauer <stepan@coresystems.de>
>>     
> Just my bad habit of not signing off on patches I haven't even tested.
>   
We'll to that for you if you keep them coming,... at least trying ;-)