arparse library assumes enum are 64 bit

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: [email protected]
          Reporter: [email protected]

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

Signature · 2 lines

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

Quoted 36 lines · On Sat, Mar 30, 2024 at 02:58:41AM +0000, [email protected] 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: [email protected]
          Reporter: [email protected]

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.

Quoted 4 lines

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.

Quoted 65 lines · On Mon, 1 Apr 2024 10:20:14 -0700 Tyler Retzlaff…

Quoted 36 lines · On Sat, Mar 30, 2024 at 02:58:41AM +0000, [email protected] 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: [email protected]
          Reporter: [email protected]

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.

Quoted 4 lines

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.

Quoted 7 lines

* /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.