From: Tyler Retzlaff <roretzla@linux.microsoft.com>
To: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Cc: "Morten Brørup" <mb@smartsharesystems.com>,
"Stephen Hemminger" <stephen@networkplumber.org>,
"techboard@dpdk.org" <techboard@dpdk.org>,
"Mattias Rönnblom" <hofors@lysator.liu.se>,
"dev@dpdk.org" <dev@dpdk.org>,
"Bruce Richardson" <bruce.richardson@intel.com>,
"Thomas Monjalon" <thomas@monjalon.net>
Subject: Re: [PATCH 0/4] RFC samples converting VLA to alloca
Date: Tue, 9 Apr 2024 08:08:34 -0700 [thread overview]
Message-ID: <20240409150834.GA30471@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <09aac29da90a499ebfc16493e9942bf6@huawei.com>
On Tue, Apr 09, 2024 at 08:28:48AM +0000, Konstantin Ananyev wrote:
>
>
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Monday, 8 April 2024 17.27
> > >
> > > For next technboard meeting.
> > >
> > > On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote:
> > > > On Sun, 7 Apr 2024 13:07:06 +0200
> > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> > > > > > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > > > > > Sent: Sunday, 7 April 2024 11.32
> > > > > >
> > > > > > On 2024-04-04 19:15, Tyler Retzlaff wrote:
> > > > > > > This series is not intended for merge. It insteat provides examples
> > > > > > of
> > > > > > > converting use of VLAs to alloca() would look like.
> > > > > > >
> > > > > > > what's the advantages of VLA over alloca()?
> > > > > > >
> > > > > > > * sizeof(array) works as expected.
> > > > > > >
> > > > > > > * multi-dimensional arrays are still arrays instead of pointers to
> > > > > > > dynamically allocated space. this means multiple subscript syntax
> > > > > > > works (unlike on a pointer) and calculation of addresses into
> > > > > > allocated
> > > > > > > space in ascending order is performed by the compiler instead of
> > > > > > manually.
> > > > > > >
> > > > > >
> > > > > > alloca() is a pretty obscure mechanism, and also not a part of the C
> > > > > > standard. VLAs are C99, and well-known and understood, and very
> > > > > > efficient.
> > > > >
> > > > > The RFC fails to mention why we need to replace VLAs with something else:
> > > > >
> > > > > VLAs are C99, but not C++; VLAs were made optional in C11.
> > > > >
> > > > > MSVC doesn't support VLAs, and is not going to:
> > > > > https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-
> > > arriving-in-msvc/#variable-length-arrays
> > > > >
> > > > >
> > > > > I dislike alloca() too, and the notes section in the alloca(3) man page
> > > even discourages the use of alloca():
> > > > > https://man7.org/linux/man-pages/man3/alloca.3.html
> > > > >
> > > > > But I guess alloca() is the simplest replacement for VLAs.
> > > > > This RFC patch series opens the discussion for alternatives in different
> > > use cases.
> > > > >
> > > >
> > > > The other issue with VLA's is that if the number is something that can be
> > > externally
> > > > input, then it can be a source of stack overflow bugs. That is why the Linux
> > > kernel
> > > > has stopped using them; for security reasons. DPDK has much less of a
> > > security
> > > > trust domain. Mostly need to make sure that no data from network is being
> > > > used to compute VLA size.
> > > >
> > >
> > > Looks like we need to discuss this at the next techboard meeting.
> > >
> > > * MSVC doesn't support C11 optional VLAs (and never will).
> > > * alloca() is an alternative that is available on all platforms/toolchain
> > > combinations.
> > > * it's reasonable for some VLAs to be turned into regular arrays but it
> > > would be unsatisfactory to be stuck waiting discussions of defining new
> > > constant expression macros on a per-use basis.
> >
> > We must generally stop using VLAs, for many reasons.
> > The only available 1:1 replacement is alloca(), so we have to accept that.
> >
> > If anyone still cares about improvements, we can turn alloca()'d arrays into regular arrays after this patch series.
> >
> > Alternatives to VLAs are very interesting discussions, but let's not stall MSVC progress because of it!
>
> Ok, but why we have to rush into 'alloca()' solution if none of us really fond of it?
for the trivial case it is no worse than a VLA. while it isn't
standardized it is available for all platform/toolchains unlike VLA.
most of the code needed to be changed for windows falls into the trivial
case when converted.
there do appear to be cases where VLAs have just been unintentional.
i previously linked a patch where i fixed a case where they were
instantiated inside a cast and there are other cases i'm aware of in the
mlx5 driver where i believe they are unintended. at least with alloca
it is obvious but with a VLA if the expression used to determine the
size is wrapped up in something non-trivial and the author doesn't check
that it is truly a constant expression you get one by surprise.
> As you already noted majority of these cases can be replaced with static sized arrays.
unfortunately i don't think this is the case if we are talking about the
entire source tree.
> Let's try to compile a list of what needs to be changed, split it by priorities and work
> progressively through it.
i agree that working progressively is the way forward, my suggestion
partitioning has been to submit a smaller series that unblocks windows
using alloca as a starting point. this represents only a fraction of the
uses but can also serve for evaluation purposes.
if maintainers can identify a reasonable conversion to static array for
any of the converted instances i can incorporate the prescribed changes.
i would also suggest that in parallel we might introduce a series that
enables -Wvla but suppresses warning about -Wvla at the sites of use.
the purpose of this suggestion is to stop new introductions but also
annotate the uses we would like maintainers to evaluate. perhaps some
could also be trivially eliminated with the series.
> Konstantin
>
> >
> > > * there is resistance to using alloca() vs VLA so my proposal is to
> > > change only the code that is built to target windows.
> >
> > I would prefer to get rid of them all, so the CI can build with -Wvla to prevent them from being introduced again.
> > Not a strong preference.
> > On the other hand, the CI's MSVC builds will catch them if used for a Windows target.
> > And limiting to Windows code reduces the amount of work, so that's probably the most realistic solution.
next prev parent reply other threads:[~2024-04-09 15:08 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-07 19:32 RFC acceptable handling of VLAs across toolchains Tyler Retzlaff
2023-11-08 2:31 ` Stephen Hemminger
2023-11-08 3:25 ` Tyler Retzlaff
2023-11-08 8:19 ` Morten Brørup
2023-11-08 16:51 ` Stephen Hemminger
2023-11-08 17:48 ` Morten Brørup
2023-11-09 10:25 ` RFC: default burst sizes in rte_config Morten Brørup
2023-11-09 20:26 ` RFC acceptable handling of VLAs across toolchains Tyler Retzlaff
2024-03-21 0:12 ` Tyler Retzlaff
2024-04-04 17:15 ` [PATCH 0/4] RFC samples converting VLA to alloca Tyler Retzlaff
2024-04-04 17:15 ` [PATCH 1/4] latencystats: use alloca instead of vla trivial Tyler Retzlaff
2024-04-06 15:28 ` Morten Brørup
2024-04-07 9:36 ` Mattias Rönnblom
2024-04-07 17:00 ` Stephen Hemminger
2024-04-04 17:15 ` [PATCH 2/4] hash: " Tyler Retzlaff
2024-04-06 16:01 ` Morten Brørup
2024-04-04 17:15 ` [PATCH 3/4] vhost: use alloca instead of vla sizeof Tyler Retzlaff
2024-04-06 22:30 ` Morten Brørup
2024-04-04 17:15 ` [PATCH 4/4] dispatcher: use alloca instead of vla multi dimensional Tyler Retzlaff
2024-04-06 15:49 ` Morten Brørup
2024-04-07 9:31 ` [PATCH 0/4] RFC samples converting VLA to alloca Mattias Rönnblom
2024-04-07 11:07 ` Morten Brørup
2024-04-07 17:03 ` Stephen Hemminger
2024-04-08 15:27 ` Tyler Retzlaff
2024-04-08 15:53 ` Morten Brørup
2024-04-09 8:28 ` Konstantin Ananyev
2024-04-09 15:08 ` Tyler Retzlaff [this message]
2024-04-10 9:58 ` Konstantin Ananyev
2024-04-10 17:03 ` Tyler Retzlaff
2024-04-10 7:32 ` Mattias Rönnblom
2024-04-10 7:52 ` Morten Brørup
2024-04-10 17:04 ` Tyler Retzlaff
2024-04-10 7:27 ` Mattias Rönnblom
2024-04-10 17:10 ` Tyler Retzlaff
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=20240409150834.GA30471@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net \
--to=roretzla@linux.microsoft.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=hofors@lysator.liu.se \
--cc=konstantin.ananyev@huawei.com \
--cc=mb@smartsharesystems.com \
--cc=stephen@networkplumber.org \
--cc=techboard@dpdk.org \
--cc=thomas@monjalon.net \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).