Re: RFC acceptable handling of VLAs across toolchains

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

So just top posting to revive this discussion.


i spent some time going through lib and drivers and the use of VLAs is
very extensive. additionally, i have learned that there is some syntax
improvement value in using them over alloca() in spite of neither being
able to report allocation failure.

i would like to propose that for msvc we only adapt code that targets
windows and to use alloca() and as for all of the non-windows built code
leave existing VLA use, perhaps adding -Wvla and suppressing the warning
on existing uses.

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 addresses of elements in the
  multi-dimensional array are in ascending order. you can approximate
  a multi-dimensional array with alloca() but it isn't one and you
  are burdened with loops for initialization and non-subscripted
  syntax to access elements making code harder to maintain.

for the above reasons i'd recommend only converting to alloca() where
necessary (msvc has to compile it) and for the other instances leave
them as they are. alternative is i could just conditionally compile
and duplicate code at usage sites which is pretty ugly.

appreciate input/feedback here.

thanks!

On Thu, Nov 09, 2023 at 12:26:48PM -0800, Tyler Retzlaff wrote:
> On Wed, Nov 08, 2023 at 08:51:54AM -0800, Stephen Hemminger wrote:
> > On Tue, 7 Nov 2023 11:32:20 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > 
> > > hi folks,
> > > 
> > > i'm seeking advice. we have use of VLAs which are now optional in
> > > standard C. some toolchains provide a conformant implementation and msvc
> > > does not (and never will).
> > > 
> > > we seem to have a few options, just curious about what people would
> > > prefer.
> > > 
> > > * use alloca
> > > 
> > > * use dynamically allocated storage
> > > 
> > > * conditional compiled code where the msvc leg uses one of the previous
> > >   two options
> > > 
> > > i'll leave it simple for now, i'd like to hear input rather than provide
> > > a recommendation for now.
> > > 
> > > thanks!
> > 
> > As an experiment did a build of current DPDK with -Wvla option.
> 
> so maybe what i will do here is put a series up that convers to alloca()
> for libs and enables -Wvla as a part of the review we can discuss
> case-by-case basis of keeping alloca or converting to regular C arrays?
> 
> for the items identified below i'll make the conversions as you have
> suggested in v1 of the series and seek further comment.
> 
> > 
> > Lots of errors, some have obvious solutions like:
> > 
> > ../drivers/net/failsafe/failsafe_intr.c: In function ‘fs_rx_event_proxy_service_install’:
> > ../drivers/net/failsafe/failsafe_intr.c:142:17: warning: ISO C90 forbids variable length array ‘service_core_list’ [-Wvla]
> >   142 |                 uint32_t service_core_list[num_service_cores];
> >       |                 ^~~~~~~~
> > 
> > This could just be RTE_MAX_LCORES.
> > 
> > others like rte_metrics should just use malloc() as is used already in
> > that function.
> > 
> > ../lib/metrics/rte_metrics_telemetry.c: In function ‘rte_metrics_tel_update_metrics_ethdev’:
> > ../lib/metrics/rte_metrics_telemetry.c:140:9: warning: ISO C90 forbids variable length array ‘xstats_values’ [-Wvla]
> >   140 |         uint64_t xstats_values[num_xstats];
> >       |         ^~~~~~~~
> > ../lib/metrics/rte_metrics_telemetry.c: In function ‘rte_metrics_tel_extract_data’:
> > ../lib/metrics/rte_metrics_telemetry.c:384:9: warning: ISO C90 forbids variable length array ‘stat_names’ [-Wvla]
> >   384 |         const char *stat_names[num_stat_names];
> > |         ^~~~~
> > 
> > Others already have an implicit upper bound.
> > Example is in rte_cuckoo_hash where some fields us RTE_HASH_LOOKUP_BULK_MAX
> > and some use VLA.
> > 
> > [170/2868] Compiling C object lib/librte_hash.a.p/hash_rte_cuckoo_hash.c.o
> > ../lib/hash/rte_cuckoo_hash.c: In function ‘rte_hash_lookup_bulk_data’:
> > ../lib/hash/rte_cuckoo_hash.c:2355:9: warning: ISO C90 forbids variable length array ‘positions’ [-Wvla]
> >  2355 |         int32_t positions[num_keys];
> >       |         ^~~~~~~
> > ../lib/hash/rte_cuckoo_hash.c: In function ‘rte_hash_lookup_with_hash_bulk_data’:
> > ../lib/hash/rte_cuckoo_hash.c:2471:9: warning: ISO C90 forbids variable length array ‘positions’ [-Wvla]
> >  2471 |         int32_t positions[num_keys];
> >       |         ^~~~~~~
> > 
> > Would it make sense to have an rte_config.h value for maximum burst size?
> > Lots of code is using nb_pkts.
> > 
> > There is also some confusing ones like:
> > ../lib/mempool/rte_mempool.c: In function ‘mempool_cache_init’:
> > ../lib/mempool/rte_mempool.c:751:50: warning: ISO C90 forbids array whose size cannot be evaluated [-Wvla]
> >   751 |                          RTE_SIZEOF_FIELD(struct rte_mempool_cache, objs[0]));
> >       |                                                  ^~~~~~~~~~~~~~~~~
> > ../lib/eal/include/rte_common.h:498:65: note: in definition of macro ‘RTE_BUILD_BUG_ON’
> >   498 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> >       |                                                                 ^~~~~~~~~
> > ../lib/mempool/rte_mempool.c:751:26: note: in expansion of macro ‘RTE_SIZEOF_FIELD’
> >   751 |                          RTE_SIZEOF_FIELD(struct rte_mempool_cache, objs[0]));