From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D135843D0B; Thu, 21 Mar 2024 01:12:16 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9820D4029F; Thu, 21 Mar 2024 01:12:16 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id CE62E4028B for ; Thu, 21 Mar 2024 01:12:14 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 1DB1B20B74C0; Wed, 20 Mar 2024 17:12:14 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1DB1B20B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1710979934; bh=RYlVysMAiRwEAAoz97OuQSyeH+LUcB0vBla01Q7dzts=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UGbaJyx6rDq3AHp3f+Gj6hXTkVNQSiXHpcP2gQiURW+zzq53BbihmOV4ghaNELz2e EpGAAtI3oLqAPzFxIApxmQ6GYslDNmmRuUszs+HadXiYj/dLBXtOBXC229BYI6z80w gyzg0bI7UjeP+9apLRcBuuD01gweZ+y1dDBYr9U4= Date: Wed, 20 Mar 2024 17:12:14 -0700 From: Tyler Retzlaff To: Stephen Hemminger Cc: dev@dpdk.org Subject: Re: RFC acceptable handling of VLAs across toolchains Message-ID: <20240321001214.GA4255@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20231107193220.GA15232@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20231108085154.757719e4@hermes.local> <20231109202648.GA7506@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20231109202648.GA7506@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 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]));