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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Mon, Apr 01, 2024 at 12:34:08PM -0700, Stephen Hemminger wrote:
> 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.

agreed.

thanks for raising it.