On Mon, Apr 01, 2024 at 12:34:08PM -0700, Stephen Hemminger wrote: > On Mon, 1 Apr 2024 10:20:14 -0700 > Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > > On Sat, Mar 30, 2024 at 02:58:41AM +0000, bugzi...@dpdk.org wrote: > > > https://bugs.dpdk.org/show_bug.cgi?id=1409 > > > > > > Bug ID: 1409 > > > Summary: arparse library assumes enum are 64 bit > > > Product: DPDK > > > Version: 24.03 > > > Hardware: All > > > OS: All > > > Status: UNCONFIRMED > > > Severity: normal > > > Priority: Normal > > > Component: core > > > Assignee: dev@dpdk.org > > > Reporter: step...@networkplumber.org > > > Target Milestone: --- > > > > > > MSVC correctly flags that this line in rte_argparse.h is incorrect: > > > RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48), > > > > > > The problem is that enum values are just an alias for int, and it can be > > > 32 > > > bits. > > > > > > Taken from the current C Standard (C99): > > > http://www.open std.org/JTC1/SC22/WG14/www/docs/n1256.pdf > > > > > > 6.7.2.2 Enumeration specifiers > > > [...] > > > Constraints > > > The expression that defines the value of an enumeration constant shall be > > > an > > > integer constant expression that has a value representable as an int. > > > [...] > > > Each enumerated type shall be compatible with char, a signed integer > > > type, or > > > an unsigned integer type. The choice of type is implementation-defined, > > > but > > > shall be capable of representing the values of all the members of the > > > enumeration. > > > > > > Since rte_argparse only uses 10 bits now. The suggested fix here is to: > > > 1. Assume 32 bits > > > 2. Get rid of the reserved field - reserved fields are bad idea > > > > > > > as some additional information i was aware of this issue and had already > > discussing it internally with the visual studio engineers. we reviewed > > relevant parts of C11 standard we believe there are 2 points of > > interest. > > > > the C11 standard does appear to direct the implementation to select an > > integer wide enough to hold the 64-bit enum value but it is only > > reasonable to do so when the target has a native 64-bit type. i.e. if > > your target has no 64-bit integer than the size of the above constant > > expression will be truncated. > > > > the MSVC compiler requires an extra command line argument to provide > > standard C conformant behavior (/Zc:enumTypes). when used with a C++ TU > > MSVC does select a 64-bit type for the prescribed constant expression > > but does not correctly select a 64-bit type with a C TU (this matters if > > we are exposing this enum type in a public header consumed by C++) > > > > i'm in the process of requesting that /Zc:enumTypes be brought into > > alignment with how it functions with C++ TU. > > > > * /Zc:enumTypes should result in "the same" type being selected for > > C or C++ TU, whatever that type may be. > > > > * /Zc:enumTypes in a C TU should select a 64-bit integer type for the > > above example value when the target supports 64-bit integer natively. > > > > as the current released compiler obviously does not conform to the above > > we may apply a conditionally compiled workaround that declares a 64-bit > > integer with an identifier matching the name of the un-scoped enum > > value. > > All well and good, but the assumption of 64 bit enum's breaks on > 32 bit builds which DPDK still has. The library didn't need the bits. > Just deleting the unused reserved field and changing the shifts to > be 32 fixes the issue.
agreed. thanks for raising it.
