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

[Eric Blake]

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. :(

And while C17 expands the list of integer constant expressions to
include _Alignof expressions, it does not add any wording to permit
const variables.

https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c
helps with this explanation:
"The thing to remember (and yes, this is a bit counterintuitive) is that
const doesn't mean constant. A constant expression is, roughly, one that
can be evaluated at compile time (like 2+2 or 42). The const type
qualifier, even though its name is obviously derived from the English
word "constant", really means "read-only".

Consider, for example, that these are a perfectly valid declarations:

const int r = rand();
const time_t now = time(NULL);

The const just means that you can't modify the value of r or now after
they've been initialized. Those values clearly cannot be determined
until execution time."

And C++ _does_ support named constants, but we're using C, not C++.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org