Re: [DPDK/core Bug 1409] arparse library assumes enum are 64 bit

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 1 Apr 2024 10:20:14 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Sat, Mar 30, 2024 at 02:58:41AM +0000, bugzilla@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: stephen@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.