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 2F4AC4296E; Mon, 17 Apr 2023 17:46:05 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 058DD40DFB; Mon, 17 Apr 2023 17:46:05 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 8575840698 for ; Mon, 17 Apr 2023 17:46:03 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id AF73621C1E62; Mon, 17 Apr 2023 08:46:02 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AF73621C1E62 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1681746362; bh=NFW/9CrQYkzhVE7CpCeLcn7vWSq6ew1ZNBw3RvZM/H0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RsIPugLISljecdj/6XgmyXomUgxolJCjYHIUKZHLMHNRxRowKyAXt87ieTGMa5D7x J05PLyGdAalgJWFRN4UBwAr3tY9pcLQCu7Mu+6qgBH9zIyYAC+Ejv1Bok7ld9GIHrz ftLrElUQx6IuVoMfvpiyR4Tg2tHw5ou3px79malw= Date: Mon, 17 Apr 2023 08:46:02 -0700 From: Tyler Retzlaff To: Konstantin Ananyev Cc: dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.net, mb@smartsharesystems.com, konstantin.ananyev@huawei.com Subject: Re: [PATCH v3 06/11] eal: typedef cpu flag enum as int for msvc Message-ID: <20230417154602.GA3180@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1680741919-22102-1-git-send-email-roretzla@linux.microsoft.com> <1680741919-22102-7-git-send-email-roretzla@linux.microsoft.com> <54a3d95b-51b3-6f3e-7d2f-d893ae9a8c4a@yandex.ru> <20230410205326.GA4798@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <5713dbf3-25d0-37c2-5996-2e6344bcee18@yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5713dbf3-25d0-37c2-5996-2e6344bcee18@yandex.ru> 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 On Sun, Apr 16, 2023 at 10:29:38PM +0100, Konstantin Ananyev wrote: > 10/04/2023 21:53, Tyler Retzlaff пишет: > >On Mon, Apr 10, 2023 at 08:59:33PM +0100, Konstantin Ananyev wrote: > >>06/04/2023 01:45, Tyler Retzlaff пишет: > >>>Forward declaration of a typedef is a non-standard extension and is not > >>>supported by msvc. Use an int instead. > >>> > >>>Abstract the use of the int/enum rte_cpu_flag_t in function parameter > >>>lists by re-typdefing the enum rte_cpu_flag_t to the rte_cpu_flag_t > >>>identifier. > >>> > >>>Remove the use of __extension__ on function prototypes where > >>>rte_cpu_flag_t appeared in parameter lists, it is sufficient to have the > >>>conditionally compiled __extension__ at the non-standard forward > >>>declaration site. > >>> > >>>Signed-off-by: Tyler Retzlaff > >>>--- > >>> lib/eal/include/generic/rte_cpuflags.h | 12 +++++++----- > >>> 1 file changed, 7 insertions(+), 5 deletions(-) > >>> > >>>diff --git a/lib/eal/include/generic/rte_cpuflags.h b/lib/eal/include/generic/rte_cpuflags.h > >>>index d35551e..87ab03c 100644 > >>>--- a/lib/eal/include/generic/rte_cpuflags.h > >>>+++ b/lib/eal/include/generic/rte_cpuflags.h > >>>@@ -44,8 +44,12 @@ struct rte_cpu_intrinsics { > >>> /** > >>> * Enumeration of all CPU features supported > >>> */ > >>>+#ifndef RTE_TOOLCHAIN_MSVC > >>> __extension__ > >>>-enum rte_cpu_flag_t; > >>>+typedef enum rte_cpu_flag_t rte_cpu_flag_t; > >>>+#else > >>>+typedef int rte_cpu_flag_t; > >>>+#endif > >> > >> > >>Just curious what exactly MSVC doesn't support here? > >>Is that construction like: > >>enum rte_cpu_flag_t {....}; > >>enum rte_cpu_flag_t; > >>... > >>Or something else? > > > >Forward declaration of an enum is non standard. It's probably only > >allowed by gcc as an extension because gcc will make some kind of > >implementation specific promise for it always to be `int` size by > >default (assuming no other -foptions). > > I understood that part, what is not clear to me from where we are > getting forward declarations? i investigated and expirmented with this further. i can see now there were two things that were tripping things up portability wise. * it's still a forward declaration even if the full enum definition is visible and thus generates a warning (but not an error). * the use of __extension__ which is valid in this context because it is after all an extension (causes compilation error for msvc). my testing shows i'm still getting correct sizes (because the definition is visible) so what i'll do is update this patch to remove the use of __extension__ and allow the warning to be emitted for now. i think this is probably what you're preference would be. in a future series i will either suppress the warning for msvc or remove the declaration entirely which as you point out should not be needed due to the include via arch/rte_cpuflags.h -> include generic/rte_cpuflags.h. thanks! > As I remember the usual organization of arch specific rte_cpuflags.h: > > /* type definition */ > enum rte_cpu_flag_t {...}; > > /some other stuff */ > > #include "generic/rte_cpuflags.h" > > Which contains 'enum rte_cpu_flag_t' type declaration. > But it doesn't look like a forward declaration to me. > Is there a place where we do include "generic/rte_cpuflags.h" directly > (not from arch specific header)? > If so. might be we should change it to include arch specific header instead? > > > > >If the enum was defined before reference it would probably be accepted > >by msvc since it could 'see' the definition and know the integer width > >in use. > > > >> > >> > >>> /** > >>> * Get name of CPU flag > >>>@@ -56,9 +60,8 @@ struct rte_cpu_intrinsics { > >>> * flag name > >>> * NULL if flag ID is invalid > >>> */ > >>>-__extension__ > >>> const char * > >>>-rte_cpu_get_flag_name(enum rte_cpu_flag_t feature); > >>>+rte_cpu_get_flag_name(rte_cpu_flag_t feature); > >>> /** > >>> * Function for checking a CPU flag availability > >>>@@ -70,9 +73,8 @@ struct rte_cpu_intrinsics { > >>> * 0 if flag is not available > >>> * -ENOENT if flag is invalid > >>> */ > >>>-__extension__ > >>> int > >>>-rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature); > >>>+rte_cpu_get_flag_enabled(rte_cpu_flag_t feature); > >>> /** > >>> * This function checks that the currently used CPU supports the CPU features