Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 5/6/21 4:15 AM, Keith Busch wrote:
> On Wed, May 05, 2021 at 06:09:10PM -0500, Eric Blake wrote:
>> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
>>> +Eric
>>>
>>> On 5/5/21 11:22 PM, Keith Busch wrote:
>>>> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
>>>>> a constant! Help it by using a definitions instead.
>>>>
>>>> I don't understand.
>>>
>>> Neither do I TBH...
>>>
>>>> It's labeled 'const', so any reasonable compiler
>>>> will place it in the 'text' segment of the executable rather than on the
>>>> stack. While that's compiler specific, is there really a compiler doing
>>>> something bad with this? If not, I do prefer the 'const' here if only
>>>> because it limits the symbol scope ('static const' is also preferred if
>>>> that helps).
>>>
>>> Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
>>>
>>> Both static+const / const trigger:
>>>
>>> hw/block/nvme.c: In function ‘nvme_map_sgl’:
>>> hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
>>> ‘segment’ [-Werror=vla]
>>>   818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>>>       |     ^~~~~~~~~~~~~~~~~
>>> cc1: all warnings being treated as errors
>>
>> C99 6.7.5.2 paragraph 4:
>> "If the size is an integer constant expression and the element type has
>> a known constant size, the array type is not a variable length array
>> type; otherwise, the array type is a variable length array type."
>>
>> 6.6 paragraph 6:
>> "An integer constant expression shall have integer type and shall only
>> have operands that are integer constants, enumeration constants,
>> character constants, sizeof expressions whose results are integer
>> constants, and floating constants that are the immediate operands of
>> casts. Cast operators in an integer constant expression shall only
>> convert arithmetic types to integer types, except as part of an operand
>> to the sizeof operator."
>>
>> Notably absent from that list are 'const int' variables, which even
>> though they act as constants (in that the name always represents the
>> same value), do not actually qualify as such under C99 rules.  Yes, it's
>> a pain.  What's more, 6.6 paragraph 10:
>>
>> "An implementation may accept other forms of constant expressions."
>>
>> which means it _should_ be possible for the compiler to do what we want.
>>  But just because it is permitted does not make it actually work. :(
> 
> Thank you, that makes sense. In this case, we are talking about an
> integer constant expression for the value of a 'const' symbol. That
> seems like perfect fodder for a compiler to optimize. I understand it's
> not obligated to do that, but I assumed it would.
> 
> Anyway, Philippe, I have no objection if you want to push forward with
> this series.

Thanks both. I'll amend Eric explanation in the commit description.

Regards,

Phil.