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 F149442971; Tue, 18 Apr 2023 00:10:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 83D4840EDF; Tue, 18 Apr 2023 00:10:31 +0200 (CEST) Received: from forward502b.mail.yandex.net (forward502b.mail.yandex.net [178.154.239.146]) by mails.dpdk.org (Postfix) with ESMTP id 1D00540E09 for ; Tue, 18 Apr 2023 00:10:30 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-39.sas.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-39.sas.yp-c.yandex.net [IPv6:2a02:6b8:c08:2087:0:640:7bf5:0]) by forward502b.mail.yandex.net (Yandex) with ESMTP id 4BDAD5E4DE; Tue, 18 Apr 2023 01:10:29 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-39.sas.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id QAk5Lx3Ww0U0-rp1TjZLv; Tue, 18 Apr 2023 01:10:28 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1681769428; bh=DaziiAqfjZMwMRwjCtiyFCNVjyJA3kdp7TXXZdfo6E8=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=QAhPuV+d+0pZInmfQdTPu/whAKcKaXtIQZrVQ4Wn8VHDh8CLLmttfCGa5xD16cDvy QxZDT1VC2wEs9kMXfIRIDBOGqIoM75BRX8YvBam1HkYb/wi6edZmxz8/gn/unTd6Bf GguIdrxiT3JTgljyxtk4RYzCNICDhYUX39yOlvLw= Authentication-Results: mail-nwsmtp-smtp-production-main-39.sas.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: Date: Mon, 17 Apr 2023 23:10:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH v3 06/11] eal: typedef cpu flag enum as int for msvc Content-Language: en-US To: Tyler Retzlaff Cc: dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.net, mb@smartsharesystems.com, konstantin.ananyev@huawei.com 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> <20230417154602.GA3180@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: Konstantin Ananyev In-Reply-To: <20230417154602.GA3180@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 17/04/2023 16:46, Tyler Retzlaff пишет: > 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. Agree, let's try to remove this declaration. I don't see any point to have declaration straight after defintion. Thanks Konstantin > > 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