All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Keith Busch" <kbusch@kernel.org>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org,
	"Richard Henderson" <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	qemu-ppc@nongnu.org, "Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Klaus Jensen" <its@irrelevant.dk>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
Date: Wed, 5 May 2021 18:09:10 -0500	[thread overview]
Message-ID: <c72ab9cd-e958-ee21-ef18-bd84c9dfd39c@redhat.com> (raw)
In-Reply-To: <285e17d3-93fb-7317-8aba-689fda772f84@redhat.com>

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



  reply	other threads:[~2021-05-05 23:10 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 01/23] block/vpc: Avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
2021-05-05 21:12   ` Samuel Thibault
2021-05-05 21:24   ` Marc-André Lureau
2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-05 21:14   ` Samuel Thibault
2021-05-05 21:27   ` Marc-André Lureau
2021-05-05 21:39     ` Samuel Thibault
2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
2021-05-05 21:15   ` Samuel Thibault
2021-05-05 21:29   ` Marc-André Lureau
2021-05-05 21:10 ` [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1 Philippe Mathieu-Daudé
2021-05-06  8:36   ` Daniel P. Berrangé
2021-05-05 21:10 ` [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-06  8:53   ` Stefan Hajnoczi
2021-05-06  9:01     ` Philippe Mathieu-Daudé
2021-05-06 14:47       ` Stefan Hajnoczi
2021-05-06 15:19         ` Philippe Mathieu-Daudé
2021-05-10  9:09           ` Stefan Hajnoczi
2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-05 21:22   ` Keith Busch
2021-05-05 22:07     ` Philippe Mathieu-Daudé
2021-05-05 23:09       ` Eric Blake [this message]
2021-05-06  0:14         ` Warner Losh
2021-05-06  2:15         ` Keith Busch
2021-05-06  6:42           ` Philippe Mathieu-Daudé
2021-05-07 16:22           ` Richard Henderson
2021-05-06  6:27   ` Klaus Jensen
2021-05-07 15:59   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 08/23] hw/block/nvme: Avoid " Philippe Mathieu-Daudé
2021-05-06  6:43   ` Klaus Jensen
2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-06  3:35   ` Jason Wang
2021-05-07 16:29   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 10/23] hw/ppc/pnv: Avoid " Philippe Mathieu-Daudé
2021-05-06  2:12   ` David Gibson
2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
2021-05-06  2:13   ` David Gibson
2021-05-06  8:22   ` Greg Kurz
2021-05-06 13:52     ` Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 12/23] hw/i386/multiboot: " Philippe Mathieu-Daudé
2021-05-07 16:27   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 13/23] hw/usb/hcd-xhci: " Philippe Mathieu-Daudé
2021-05-07 16:34   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-07 16:39   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
2021-05-06  2:15   ` David Gibson
2021-05-06  7:09   ` Jason Wang
2021-05-05 21:10 ` [PATCH 16/23] ui/curses: " Philippe Mathieu-Daudé
2021-05-07 16:42   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 17/23] ui/spice-display: " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid " Philippe Mathieu-Daudé
2021-05-07 16:46   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 19/23] ui/vnc-enc-tight: Avoid " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 20/23] util/iov: " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 21/23] target/ppc/kvm: " Philippe Mathieu-Daudé
2021-05-05 21:10   ` Philippe Mathieu-Daudé
2021-05-06  2:16   ` David Gibson
2021-05-06  2:16     ` David Gibson
2021-05-05 21:10 ` [PATCH 22/23] tests/unit/test-vmstate: " Philippe Mathieu-Daudé
2021-05-07 16:52   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG Philippe Mathieu-Daudé
2021-05-07 16:56   ` Richard Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c72ab9cd-e958-ee21-ef18-bd84c9dfd39c@redhat.com \
    --to=eblake@redhat.com \
    --cc=berrange@redhat.com \
    --cc=its@irrelevant.dk \
    --cc=kbusch@kernel.org \
    --cc=kraxel@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.