[libgfortran] PR 60324 Unbounded stack allocations in libgfortran

Cesar Philippidis wrote:
> On 11/13/2014 02:32 AM, Janne Blomqvist wrote:
> I hit an error when building intrinsics/random.c:
>    error: expression in static assertion is not constant
> Joseph told me that static const variables cannot be used in constant
> expressions in C, so I've replaced the _Static_assert with a regular
> assert. Are you using g++ to build libgfortran?

I wonder why you are seeing this while others aren't. It seemed to work 
for me, gcc-testresults looks fine and there weren't complaints 
yesterday or today. (The patch was committed 34 h ago.)

The default build should use gcc and not g++.

> I don't have a good baseline test this patch thoroughly, but at least I
> can bootstrap gcc without it failing in libgfortran. Is this OK for
> mainline and/or could someone see if it causes any regressions?

I think instead of doing a run-time check I'd prefer something like the 
following, keeping the compile-time assert.

+#define KISS_SIZE ((GFC_INTEGER_4) (sizeof(kiss_seed)/sizeof(kiss_seed[0]))

(plus s/kiss_size/KISS_SIZE/ changes in the code.)

Janne, what do you think?

Tobias

On Fri, Nov 14, 2014 at 11:02 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Cesar Philippidis wrote:
>>
>> On 11/13/2014 02:32 AM, Janne Blomqvist wrote:
>> I hit an error when building intrinsics/random.c:
>>    error: expression in static assertion is not constant
>> Joseph told me that static const variables cannot be used in constant
>> expressions in C, so I've replaced the _Static_assert with a regular
>> assert. Are you using g++ to build libgfortran?
>
>
> I wonder why you are seeing this while others aren't.

Yeah, I wonder the same?

>> I don't have a good baseline test this patch thoroughly, but at least I
>> can bootstrap gcc without it failing in libgfortran. Is this OK for
>> mainline and/or could someone see if it causes any regressions?
>
>
> I think instead of doing a run-time check I'd prefer something like the
> following, keeping the compile-time assert.
>
> --- a/libgfortran/intrinsics/random.c
> +++ b/libgfortran/intrinsics/random.c
> @@ -253 +253 @@ static GFC_UINTEGER_4 kiss_default_seed[] = {
> -static const GFC_INTEGER_4 kiss_size =
> sizeof(kiss_seed)/sizeof(kiss_seed[0]);
> +#define KISS_SIZE ((GFC_INTEGER_4) (sizeof(kiss_seed)/sizeof(kiss_seed[0]))
>
> (plus s/kiss_size/KISS_SIZE/ changes in the code.)
>
> Janne, what do you think?

I like it. With this, you can also get rid of the assert and the newly
introduced KISS_MAX_SIZE macro, and just make the seed array the
correct size, as was originally done (with a VLA). Consider such a
patch pre-approved.

On 11/14/2014 10:01 PM, Janne Blomqvist wrote:
> On Fri, Nov 14, 2014 at 11:02 PM, Tobias Burnus <burnus@net-b.de> wrote:
>> Cesar Philippidis wrote:
>>>
>>> On 11/13/2014 02:32 AM, Janne Blomqvist wrote:
>>> I hit an error when building intrinsics/random.c:
>>>    error: expression in static assertion is not constant
>>> Joseph told me that static const variables cannot be used in constant
>>> expressions in C, so I've replaced the _Static_assert with a regular
>>> assert. Are you using g++ to build libgfortran?
>>
>>
>> I wonder why you are seeing this while others aren't.
> 
> Yeah, I wonder the same?

It does seem strange. Maybe that function isn't necessary for all
targets? I've attached a reduced test case if anyone is interested in
it. That error should show up with "gcc random.c".

>>> I don't have a good baseline test this patch thoroughly, but at least I
>>> can bootstrap gcc without it failing in libgfortran. Is this OK for
>>> mainline and/or could someone see if it causes any regressions?
>>
>>
>> I think instead of doing a run-time check I'd prefer something like the
>> following, keeping the compile-time assert.
>>
>> --- a/libgfortran/intrinsics/random.c
>> +++ b/libgfortran/intrinsics/random.c
>> @@ -253 +253 @@ static GFC_UINTEGER_4 kiss_default_seed[] = {
>> -static const GFC_INTEGER_4 kiss_size =
>> sizeof(kiss_seed)/sizeof(kiss_seed[0]);
>> +#define KISS_SIZE ((GFC_INTEGER_4) (sizeof(kiss_seed)/sizeof(kiss_seed[0]))
>>
>> (plus s/kiss_size/KISS_SIZE/ changes in the code.)
>>
>> Janne, what do you think?
> 
> I like it. With this, you can also get rid of the assert and the newly
> introduced KISS_MAX_SIZE macro, and just make the seed array the
> correct size, as was originally done (with a VLA). Consider such a
> patch pre-approved.

Thanks for fixing this!

Cesar