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

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

[bugzilla]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

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

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #2: Type: text/html, Size: 3376 bytes --]

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

[Tyler Retzlaff]

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.

ty

> -- 
> You are receiving this mail because:
> You are the assignee for the bug.

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

[Stephen Hemminger]

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.

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

[Tyler Retzlaff]

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.