diff mbox series

[v3,08/10] eal/windows: fix rte_page_sizes with Clang on Windows

Message ID 20200414194426.1640704-9-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Windows basic memory management |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk April 14, 2020, 7:44 p.m. UTC 
  Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
Enum rte_page_size has members valued above this limit, which get
wrapped to zero, resulting in compilation error (duplicate values in
enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.

Define these values outside of the enum for Clang on Windows only.
This does not affect runtime, because Windows doesn't run on machines
with 4GiB and 16GiB hugepages.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/include/rte_memory.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jerin Jacob April 15, 2020, 9:34 a.m. UTC | #1 
On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
> Enum rte_page_size has members valued above this limit, which get
> wrapped to zero, resulting in compilation error (duplicate values in
> enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
>
> Define these values outside of the enum for Clang on Windows only.
> This does not affect runtime, because Windows doesn't run on machines
> with 4GiB and 16GiB hugepages.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  lib/librte_eal/include/rte_memory.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
> index 1b7c3e5df..3ec673f51 100644
> --- a/lib/librte_eal/include/rte_memory.h
> +++ b/lib/librte_eal/include/rte_memory.h
> @@ -34,8 +34,14 @@ enum rte_page_sizes {
>         RTE_PGSIZE_256M  = 1ULL << 28,
>         RTE_PGSIZE_512M  = 1ULL << 29,
>         RTE_PGSIZE_1G    = 1ULL << 30,
> +/* Work around Clang on Windows being limited to 32-bit underlying type. */

It does look like "enum rte_page_sizes" NOT used as enum anywhere.

[master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {

Why not remove this workaround and define all items as #define to
avoid below ifdef clutter.

> +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)

See above.

>         RTE_PGSIZE_4G    = 1ULL << 32,
>         RTE_PGSIZE_16G   = 1ULL << 34,
> +#else
> +#define RTE_PGSIZE_4G  (1ULL << 32)
> +#define RTE_PGSIZE_16G (1ULL << 34)
> +#endif
>  };
>
>  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
> --
> 2.25.1
>
Dmitry Kozlyuk April 15, 2020, 10:32 a.m. UTC | #2 
> On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
> > Enum rte_page_size has members valued above this limit, which get
> > wrapped to zero, resulting in compilation error (duplicate values in
> > enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
> >
> > Define these values outside of the enum for Clang on Windows only.
> > This does not affect runtime, because Windows doesn't run on machines
> > with 4GiB and 16GiB hugepages.
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >  lib/librte_eal/include/rte_memory.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
> > index 1b7c3e5df..3ec673f51 100644
> > --- a/lib/librte_eal/include/rte_memory.h
> > +++ b/lib/librte_eal/include/rte_memory.h
> > @@ -34,8 +34,14 @@ enum rte_page_sizes {
> >         RTE_PGSIZE_256M  = 1ULL << 28,
> >         RTE_PGSIZE_512M  = 1ULL << 29,
> >         RTE_PGSIZE_1G    = 1ULL << 30,
> > +/* Work around Clang on Windows being limited to 32-bit underlying type. */  
> 
> It does look like "enum rte_page_sizes" NOT used as enum anywhere.
> 
> [master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
> lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {
> 
> Why not remove this workaround and define all items as #define to
> avoid below ifdef clutter.
> 
> > +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)  
> 
> See above.
> 
> >         RTE_PGSIZE_4G    = 1ULL << 32,
> >         RTE_PGSIZE_16G   = 1ULL << 34,
> > +#else
> > +#define RTE_PGSIZE_4G  (1ULL << 32)
> > +#define RTE_PGSIZE_16G (1ULL << 34)
> > +#endif
> >  };
> >
> >  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
> > --
> > 2.25.1
> >  

This is a public header and removing enum rte_page_sizes will break API.
Moving members out of enum while keeping enum itself might break compilation
because of integer constants being converted to enum (with -Werror).
Jerin Jacob April 15, 2020, 10:57 a.m. UTC | #3 
On Wed, Apr 15, 2020 at 4:02 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> > On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > >
> > > Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
> > > Enum rte_page_size has members valued above this limit, which get
> > > wrapped to zero, resulting in compilation error (duplicate values in
> > > enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
> > >
> > > Define these values outside of the enum for Clang on Windows only.
> > > This does not affect runtime, because Windows doesn't run on machines
> > > with 4GiB and 16GiB hugepages.
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > ---
> > >  lib/librte_eal/include/rte_memory.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
> > > index 1b7c3e5df..3ec673f51 100644
> > > --- a/lib/librte_eal/include/rte_memory.h
> > > +++ b/lib/librte_eal/include/rte_memory.h
> > > @@ -34,8 +34,14 @@ enum rte_page_sizes {
> > >         RTE_PGSIZE_256M  = 1ULL << 28,
> > >         RTE_PGSIZE_512M  = 1ULL << 29,
> > >         RTE_PGSIZE_1G    = 1ULL << 30,
> > > +/* Work around Clang on Windows being limited to 32-bit underlying type. */
> >
> > It does look like "enum rte_page_sizes" NOT used as enum anywhere.
> >
> > [master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
> > lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {
> >
> > Why not remove this workaround and define all items as #define to
> > avoid below ifdef clutter.
> >
> > > +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)
> >
> > See above.
> >
> > >         RTE_PGSIZE_4G    = 1ULL << 32,
> > >         RTE_PGSIZE_16G   = 1ULL << 34,
> > > +#else
> > > +#define RTE_PGSIZE_4G  (1ULL << 32)
> > > +#define RTE_PGSIZE_16G (1ULL << 34)
> > > +#endif
> > >  };
> > >
> > >  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
> > > --
> > > 2.25.1
> > >
>
> This is a public header and removing enum rte_page_sizes will break API.
> Moving members out of enum while keeping enum itself might break compilation
> because of integer constants being converted to enum (with -Werror).

If none of the public API is using this enum then I think, we may not
need to make this enum as public.
Since it has ULL, I believe both cases(enum or define), it will be
treated as unsigned long long. ie. NO ABI breakage.


>
> --
> Dmitry Kozlyuk
Dmitry Kozlyuk April 15, 2020, 11:09 a.m. UTC | #4 
> On Wed, Apr 15, 2020 at 4:02 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >  
> > > On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:  
> > > >
> > > > Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
> > > > Enum rte_page_size has members valued above this limit, which get
> > > > wrapped to zero, resulting in compilation error (duplicate values in
> > > > enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
> > > >
> > > > Define these values outside of the enum for Clang on Windows only.
> > > > This does not affect runtime, because Windows doesn't run on machines
> > > > with 4GiB and 16GiB hugepages.
> > > >
> > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > ---
> > > >  lib/librte_eal/include/rte_memory.h | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
> > > > index 1b7c3e5df..3ec673f51 100644
> > > > --- a/lib/librte_eal/include/rte_memory.h
> > > > +++ b/lib/librte_eal/include/rte_memory.h
> > > > @@ -34,8 +34,14 @@ enum rte_page_sizes {
> > > >         RTE_PGSIZE_256M  = 1ULL << 28,
> > > >         RTE_PGSIZE_512M  = 1ULL << 29,
> > > >         RTE_PGSIZE_1G    = 1ULL << 30,
> > > > +/* Work around Clang on Windows being limited to 32-bit underlying type. */  
> > >
> > > It does look like "enum rte_page_sizes" NOT used as enum anywhere.
> > >
> > > [master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
> > > lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {
> > >
> > > Why not remove this workaround and define all items as #define to
> > > avoid below ifdef clutter.
> > >  
> > > > +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)  
> > >
> > > See above.
> > >  
> > > >         RTE_PGSIZE_4G    = 1ULL << 32,
> > > >         RTE_PGSIZE_16G   = 1ULL << 34,
> > > > +#else
> > > > +#define RTE_PGSIZE_4G  (1ULL << 32)
> > > > +#define RTE_PGSIZE_16G (1ULL << 34)
> > > > +#endif
> > > >  };
> > > >
> > > >  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
> > > > --
> > > > 2.25.1
> > > >  
> >
> > This is a public header and removing enum rte_page_sizes will break API.
> > Moving members out of enum while keeping enum itself might break compilation
> > because of integer constants being converted to enum (with -Werror).  
> 
> If none of the public API is using this enum then I think, we may not
> need to make this enum as public.

Agreed.

> Since it has ULL, I believe both cases(enum or define), it will be
> treated as unsigned long long. ie. NO ABI breakage.

I was talking about API only (compile-time compatibility). Getting rid of
#ifdef and workarounds sounds right, we'll just need a notice in release
notes.
Jerin Jacob April 15, 2020, 11:17 a.m. UTC | #5 
On Wed, Apr 15, 2020 at 4:39 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
>
>
> > On Wed, Apr 15, 2020 at 4:02 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > >
> > > > On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > > > >
> > > > > Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
> > > > > Enum rte_page_size has members valued above this limit, which get
> > > > > wrapped to zero, resulting in compilation error (duplicate values in
> > > > > enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
> > > > >
> > > > > Define these values outside of the enum for Clang on Windows only.
> > > > > This does not affect runtime, because Windows doesn't run on machines
> > > > > with 4GiB and 16GiB hugepages.
> > > > >
> > > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > > ---
> > > > >  lib/librte_eal/include/rte_memory.h | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
> > > > > index 1b7c3e5df..3ec673f51 100644
> > > > > --- a/lib/librte_eal/include/rte_memory.h
> > > > > +++ b/lib/librte_eal/include/rte_memory.h
> > > > > @@ -34,8 +34,14 @@ enum rte_page_sizes {
> > > > >         RTE_PGSIZE_256M  = 1ULL << 28,
> > > > >         RTE_PGSIZE_512M  = 1ULL << 29,
> > > > >         RTE_PGSIZE_1G    = 1ULL << 30,
> > > > > +/* Work around Clang on Windows being limited to 32-bit underlying type. */
> > > >
> > > > It does look like "enum rte_page_sizes" NOT used as enum anywhere.
> > > >
> > > > [master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
> > > > lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {
> > > >
> > > > Why not remove this workaround and define all items as #define to
> > > > avoid below ifdef clutter.
> > > >
> > > > > +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)
> > > >
> > > > See above.
> > > >
> > > > >         RTE_PGSIZE_4G    = 1ULL << 32,
> > > > >         RTE_PGSIZE_16G   = 1ULL << 34,
> > > > > +#else
> > > > > +#define RTE_PGSIZE_4G  (1ULL << 32)
> > > > > +#define RTE_PGSIZE_16G (1ULL << 34)
> > > > > +#endif
> > > > >  };
> > > > >
> > > > >  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
> > > > > --
> > > > > 2.25.1
> > > > >
> > >
> > > This is a public header and removing enum rte_page_sizes will break API.
> > > Moving members out of enum while keeping enum itself might break compilation
> > > because of integer constants being converted to enum (with -Werror).
> >
> > If none of the public API is using this enum then I think, we may not
> > need to make this enum as public.
>
> Agreed.
>
> > Since it has ULL, I believe both cases(enum or define), it will be
> > treated as unsigned long long. ie. NO ABI breakage.
>
> I was talking about API only (compile-time compatibility). Getting rid of
> #ifdef and workarounds sounds right, we'll just need a notice in release
> notes.

Good to check ./devtools/check-abi.sh for any ABI breakage.

>
> --
> Dmitry Kozlyuk
Ray Kinsella May 6, 2020, 5:41 a.m. UTC | #6 
On 15/04/2020 12:17, Jerin Jacob wrote:
> On Wed, Apr 15, 2020 at 4:39 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>>
>>
>>
>>> On Wed, Apr 15, 2020 at 4:02 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>>>>
>>>>> On Wed, Apr 15, 2020 at 1:16 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>>>>>>
>>>>>> Clang on Windows follows MS ABI where enum values are limited to 2^31-1.
>>>>>> Enum rte_page_size has members valued above this limit, which get
>>>>>> wrapped to zero, resulting in compilation error (duplicate values in
>>>>>> enum). Using MS ABI is mandatory for Windows EAL to call Win32 APIs.
>>>>>>
>>>>>> Define these values outside of the enum for Clang on Windows only.
>>>>>> This does not affect runtime, because Windows doesn't run on machines
>>>>>> with 4GiB and 16GiB hugepages.
>>>>>>
>>>>>> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>>>>>> ---
>>>>>>  lib/librte_eal/include/rte_memory.h | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
>>>>>> index 1b7c3e5df..3ec673f51 100644
>>>>>> --- a/lib/librte_eal/include/rte_memory.h
>>>>>> +++ b/lib/librte_eal/include/rte_memory.h
>>>>>> @@ -34,8 +34,14 @@ enum rte_page_sizes {
>>>>>>         RTE_PGSIZE_256M  = 1ULL << 28,
>>>>>>         RTE_PGSIZE_512M  = 1ULL << 29,
>>>>>>         RTE_PGSIZE_1G    = 1ULL << 30,
>>>>>> +/* Work around Clang on Windows being limited to 32-bit underlying type. */
>>>>>
>>>>> It does look like "enum rte_page_sizes" NOT used as enum anywhere.
>>>>>
>>>>> [master][dpdk.org] $ grep -ri "enum rte_page_sizes" lib/
>>>>> lib/librte_eal/include/rte_memory.h:enum rte_page_sizes {
>>>>>
>>>>> Why not remove this workaround and define all items as #define to
>>>>> avoid below ifdef clutter.
>>>>>
>>>>>> +#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)
>>>>>
>>>>> See above.
>>>>>
>>>>>>         RTE_PGSIZE_4G    = 1ULL << 32,
>>>>>>         RTE_PGSIZE_16G   = 1ULL << 34,
>>>>>> +#else
>>>>>> +#define RTE_PGSIZE_4G  (1ULL << 32)
>>>>>> +#define RTE_PGSIZE_16G (1ULL << 34)
>>>>>> +#endif
>>>>>>  };
>>>>>>
>>>>>>  #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>
>>>> This is a public header and removing enum rte_page_sizes will break API.
>>>> Moving members out of enum while keeping enum itself might break compilation
>>>> because of integer constants being converted to enum (with -Werror).
>>>
>>> If none of the public API is using this enum then I think, we may not
>>> need to make this enum as public.
>>
>> Agreed.
>>
>>> Since it has ULL, I believe both cases(enum or define), it will be
>>> treated as unsigned long long. ie. NO ABI breakage.
>>
>> I was talking about API only (compile-time compatibility). Getting rid of
>> #ifdef and workarounds sounds right, we'll just need a notice in release
>> notes.
> 
> Good to check ./devtools/check-abi.sh for any ABI breakage.

or something like this to cover all the bases.
DPDK_ABI_REF_DIR=/build/dpdk/reference/ DPDK_ABI_REF_VERSION=v20.02 ./devtools/test-meson-builds.sh


>>
>> --
>> Dmitry Kozlyuk
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_memory.h b/lib/librte_eal/include/rte_memory.h
index 1b7c3e5df..3ec673f51 100644
--- a/lib/librte_eal/include/rte_memory.h
+++ b/lib/librte_eal/include/rte_memory.h
@@ -34,8 +34,14 @@  enum rte_page_sizes {
 	RTE_PGSIZE_256M  = 1ULL << 28,
 	RTE_PGSIZE_512M  = 1ULL << 29,
 	RTE_PGSIZE_1G    = 1ULL << 30,
+/* Work around Clang on Windows being limited to 32-bit underlying type. */
+#if !defined(RTE_CC_CLANG) || !defined(RTE_EXEC_ENV_WINDOWS)
 	RTE_PGSIZE_4G    = 1ULL << 32,
 	RTE_PGSIZE_16G   = 1ULL << 34,
+#else
+#define RTE_PGSIZE_4G  (1ULL << 32)
+#define RTE_PGSIZE_16G (1ULL << 34)
+#endif
 };
 
 #define SOCKET_ID_ANY -1                    /**< Any NUMA socket. */