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

From: Keith Busch <kbusch@kernel.org>
To: Eric Blake <eblake@redhat.com>
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>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
Date: Wed, 5 May 2021 19:15:56 -0700	[thread overview]
Message-ID: <20210506021556.GA1187168@dhcp-10-100-145-180.wdc.com> (raw)
In-Reply-To: <c72ab9cd-e958-ee21-ef18-bd84c9dfd39c@redhat.com>

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.