diff mbox series

[v3,1/4] eal: disable function versioning on Windows

Message ID 20200622115528.22928-2-fady@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series build mempool on Windows |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

Fady Bader June 22, 2020, 11:55 a.m. UTC 
  Function versioning is not needed on Windows, also the function versioning
implementation is not supported by Windows.
Function versioning was disabled on Windows.

Signed-off-by: Fady Bader <fady@mellanox.com>
---
 lib/librte_eal/include/rte_function_versioning.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Thomas Monjalon June 29, 2020, 11:26 p.m. UTC | #1 
22/06/2020 13:55, Fady Bader:
> Function versioning is not needed on Windows, also the function versioning
> implementation is not supported by Windows.

I think it was said that "not needed" is not right.
Let's just say that it is not supported for now.

> Function versioning was disabled on Windows.
> 
> Signed-off-by: Fady Bader <fady@mellanox.com>
> ---
> --- a/lib/librte_eal/include/rte_function_versioning.h
> +++ b/lib/librte_eal/include/rte_function_versioning.h
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#undef RTE_BUILD_SHARED_LIB
> +#endif

This trick is a bit weird but works.
A comment is missing to explain that function versioning is disabled
but shared libraries are allowed for Windows.
Ray Kinsella June 30, 2020, 10:49 a.m. UTC | #2 
On 22/06/2020 12:55, Fady Bader wrote:
> Function versioning is not needed on Windows, also the function versioning
> implementation is not supported by Windows.
> Function versioning was disabled on Windows.
> 
> Signed-off-by: Fady Bader <fady@mellanox.com>
> ---
>  lib/librte_eal/include/rte_function_versioning.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_eal/include/rte_function_versioning.h b/lib/librte_eal/include/rte_function_versioning.h
> index f588f2643b..cee06602e9 100644
> --- a/lib/librte_eal/include/rte_function_versioning.h
> +++ b/lib/librte_eal/include/rte_function_versioning.h
> @@ -11,6 +11,10 @@
>  #error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
Do we need a meson error/rule that catches if people explicitly try to enable function_versioning. 
We don't want to just silently fail under such circumstances? 

Would something in lib/meson.build would work better?

if use_function_versioning and target_machine.system == "Windows"
	// complain loudly ... 	
endif 

>  #endif
>  
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#undef RTE_BUILD_SHARED_LIB
> +#endif
> +
>  #ifdef RTE_BUILD_SHARED_LIB
>  
>  /*
>
Fady Bader July 2, 2020, 10:25 a.m. UTC | #3 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, June 30, 2020 2:26 AM
> To: Fady Bader <fady@mellanox.com>
> Cc: dev@dpdk.org; Tasnim Bashar <tbashar@mellanox.com>; Tal Shnaiderman
> <talshn@mellanox.com>; Yohad Tor <yohadt@mellanox.com>;
> dmitry.kozliuk@gmail.com; harini.ramakrishnan@microsoft.com;
> ocardona@microsoft.com; pallavi.kadam@intel.com; ranjit.menon@intel.com;
> olivier.matz@6wind.com; arybchenko@solarflare.com; mdr@ashroe.eu;
> nhorman@tuxdriver.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] eal: disable function versioning on
> Windows
> 
> 22/06/2020 13:55, Fady Bader:
> > Function versioning is not needed on Windows, also the function
> > versioning implementation is not supported by Windows.
> 
> I think it was said that "not needed" is not right.
> Let's just say that it is not supported for now.

Ok, I'll change.

> 
> > Function versioning was disabled on Windows.
> >
> > Signed-off-by: Fady Bader <fady@mellanox.com>
> > ---
> > --- a/lib/librte_eal/include/rte_function_versioning.h
> > +++ b/lib/librte_eal/include/rte_function_versioning.h
> > +#ifdef RTE_EXEC_ENV_WINDOWS
> > +#undef RTE_BUILD_SHARED_LIB
> > +#endif
> 
> This trick is a bit weird but works.
> A comment is missing to explain that function versioning is disabled but shared
> libraries are allowed for Windows.

Sure, I'll add a comment to explain.

> 
>
Ray Kinsella July 2, 2020, 2:56 p.m. UTC | #4 
Fady,

Any comments on the below?

Ray K

On 30/06/2020 11:49, Kinsella, Ray wrote:
> 
> 
> On 22/06/2020 12:55, Fady Bader wrote:
>> Function versioning is not needed on Windows, also the function versioning
>> implementation is not supported by Windows.
>> Function versioning was disabled on Windows.
>>
>> Signed-off-by: Fady Bader <fady@mellanox.com>
>> ---
>>  lib/librte_eal/include/rte_function_versioning.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/librte_eal/include/rte_function_versioning.h b/lib/librte_eal/include/rte_function_versioning.h
>> index f588f2643b..cee06602e9 100644
>> --- a/lib/librte_eal/include/rte_function_versioning.h
>> +++ b/lib/librte_eal/include/rte_function_versioning.h
>> @@ -11,6 +11,10 @@
>>  #error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
> Do we need a meson error/rule that catches if people explicitly try to enable function_versioning. 
> We don't want to just silently fail under such circumstances? 
> 
> Would something in lib/meson.build would work better?
> 
> if use_function_versioning and target_machine.system == "Windows"
> 	// complain loudly ... 	
> endif 
> 
>>  #endif
>>  
>> +#ifdef RTE_EXEC_ENV_WINDOWS
>> +#undef RTE_BUILD_SHARED_LIB
>> +#endif
>> +
>>  #ifdef RTE_BUILD_SHARED_LIB
>>  
>>  /*
>>
Fady Bader July 5, 2020, 7 a.m. UTC | #5 
> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Thursday, July 2, 2020 5:57 PM
> To: Fady Bader <fady@mellanox.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>; Tasnim Bashar
> <tbashar@mellanox.com>; Tal Shnaiderman <talshn@mellanox.com>; Yohad Tor
> <yohadt@mellanox.com>; dmitry.kozliuk@gmail.com;
> harini.ramakrishnan@microsoft.com; ocardona@microsoft.com;
> pallavi.kadam@intel.com; ranjit.menon@intel.com; olivier.matz@6wind.com;
> arybchenko@solarflare.com; nhorman@tuxdriver.com
> Subject: Re: [PATCH v3 1/4] eal: disable function versioning on Windows
> 
> Fady,
> 
> Any comments on the below?
> 
> Ray K
> 
> On 30/06/2020 11:49, Kinsella, Ray wrote:
> >
> >
> > On 22/06/2020 12:55, Fady Bader wrote:
> >> Function versioning is not needed on Windows, also the function
> >> versioning implementation is not supported by Windows.
> >> Function versioning was disabled on Windows.
> >>
> >> Signed-off-by: Fady Bader <fady@mellanox.com>
> >> ---
> >>  lib/librte_eal/include/rte_function_versioning.h | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/lib/librte_eal/include/rte_function_versioning.h
> >> b/lib/librte_eal/include/rte_function_versioning.h
> >> index f588f2643b..cee06602e9 100644
> >> --- a/lib/librte_eal/include/rte_function_versioning.h
> >> +++ b/lib/librte_eal/include/rte_function_versioning.h
> >> @@ -11,6 +11,10 @@
> >>  #error Use of function versioning disabled, is "use_function_versioning=true"
> in meson.build?
> > Do we need a meson error/rule that catches if people explicitly try to enable
> function_versioning.
> > We don't want to just silently fail under such circumstances?
> >
> > Would something in lib/meson.build would work better?
> >
> > if use_function_versioning and target_machine.system == "Windows"
> > 	// complain loudly ...
> > endif
> >

I think this would be a better approach. 
But instead of checking if the use of function versioning is enabled under Windows
and then complain, I think we should simply disable it from Windows in the 
meson.build file. Something like this:

if target_machine.system == "Windows"
	use_function_versioning = false
endif

> >>  #endif
> >>
> >> +#ifdef RTE_EXEC_ENV_WINDOWS
> >> +#undef RTE_BUILD_SHARED_LIB
> >> +#endif
> >> +
> >>  #ifdef RTE_BUILD_SHARED_LIB
> >>
> >>  /*
> >>
Thomas Monjalon July 5, 2020, 8:37 a.m. UTC | #6 
05/07/2020 09:00, Fady Bader:
> > On 30/06/2020 11:49, Kinsella, Ray wrote:
> > > On 22/06/2020 12:55, Fady Bader wrote:
> > >> --- a/lib/librte_eal/include/rte_function_versioning.h
> > >> +++ b/lib/librte_eal/include/rte_function_versioning.h
> > >> @@ -11,6 +11,10 @@
> > >>  #error Use of function versioning disabled, is "use_function_versioning=true"
> > in meson.build?
> > > Do we need a meson error/rule that catches if people explicitly try to enable
> > function_versioning.
> > > We don't want to just silently fail under such circumstances?
> > >
> > > Would something in lib/meson.build would work better?
> > >
> > > if use_function_versioning and target_machine.system == "Windows"
> > > 	// complain loudly ...
> > > endif
> > >
> 
> I think this would be a better approach. 
> But instead of checking if the use of function versioning is enabled under Windows
> and then complain, I think we should simply disable it from Windows in the 
> meson.build file. Something like this:
> 
> if target_machine.system == "Windows"
> 	use_function_versioning = false
> endif

Did you try?

If you disable function versioning, compilation will fail
with the #error message above, right?
Fady Bader July 5, 2020, 8:51 a.m. UTC | #7 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Sunday, July 5, 2020 11:38 AM
> To: Kinsella, Ray <mdr@ashroe.eu>; Fady Bader <fady@mellanox.com>
> Cc: dev@dpdk.org; Tasnim Bashar <tbashar@mellanox.com>; Tal Shnaiderman
> <talshn@mellanox.com>; Yohad Tor <yohadt@mellanox.com>;
> dmitry.kozliuk@gmail.com; harini.ramakrishnan@microsoft.com;
> ocardona@microsoft.com; pallavi.kadam@intel.com; ranjit.menon@intel.com;
> olivier.matz@6wind.com; arybchenko@solarflare.com; nhorman@tuxdriver.com
> Subject: Re: [PATCH v3 1/4] eal: disable function versioning on Windows
> 
> 05/07/2020 09:00, Fady Bader:
> > > On 30/06/2020 11:49, Kinsella, Ray wrote:
> > > > On 22/06/2020 12:55, Fady Bader wrote:
> > > >> --- a/lib/librte_eal/include/rte_function_versioning.h
> > > >> +++ b/lib/librte_eal/include/rte_function_versioning.h
> > > >> @@ -11,6 +11,10 @@
> > > >>  #error Use of function versioning disabled, is
> "use_function_versioning=true"
> > > in meson.build?
> > > > Do we need a meson error/rule that catches if people explicitly
> > > > try to enable
> > > function_versioning.
> > > > We don't want to just silently fail under such circumstances?
> > > >
> > > > Would something in lib/meson.build would work better?
> > > >
> > > > if use_function_versioning and target_machine.system == "Windows"
> > > > 	// complain loudly ...
> > > > endif
> > > >
> >
> > I think this would be a better approach.
> > But instead of checking if the use of function versioning is enabled
> > under Windows and then complain, I think we should simply disable it
> > from Windows in the meson.build file. Something like this:
> >
> > if target_machine.system == "Windows"
> > 	use_function_versioning = false
> > endif
> 
> Did you try?
> 
> If you disable function versioning, compilation will fail with the #error message
> above, right?
>

Yes, the compilation fails, we can also change the rte_function_versioning.h
code in order not to fail under Windows.
What do you think ?
Thomas Monjalon July 5, 2020, 9 a.m. UTC | #8 
05/07/2020 10:51, Fady Bader:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 05/07/2020 09:00, Fady Bader:
> > > > On 30/06/2020 11:49, Kinsella, Ray wrote:
> > > > > On 22/06/2020 12:55, Fady Bader wrote:
> > > > >> --- a/lib/librte_eal/include/rte_function_versioning.h
> > > > >> +++ b/lib/librte_eal/include/rte_function_versioning.h
> > > > >> @@ -11,6 +11,10 @@
> > > > >>  #error Use of function versioning disabled, is
> > "use_function_versioning=true"
> > > > in meson.build?
> > > > > Do we need a meson error/rule that catches if people explicitly
> > > > > try to enable
> > > > function_versioning.
> > > > > We don't want to just silently fail under such circumstances?
> > > > >
> > > > > Would something in lib/meson.build would work better?
> > > > >
> > > > > if use_function_versioning and target_machine.system == "Windows"
> > > > > 	// complain loudly ...
> > > > > endif
> > > > >
> > >
> > > I think this would be a better approach.
> > > But instead of checking if the use of function versioning is enabled
> > > under Windows and then complain, I think we should simply disable it
> > > from Windows in the meson.build file. Something like this:
> > >
> > > if target_machine.system == "Windows"
> > > 	use_function_versioning = false
> > > endif
> > 
> > Did you try?
> > 
> > If you disable function versioning, compilation will fail with the #error message
> > above, right?
> >
> 
> Yes, the compilation fails, we can also change the rte_function_versioning.h
> code in order not to fail under Windows.
> What do you think ?

Function versioning CANNOT be supported currently on Windows.
Function versioning macros are MANDATORY in some libraries.
That's why I think the only path is to have a specific
implementation of the function versioning macros for Windows.
Thomas Monjalon July 5, 2020, 8 p.m. UTC | #9 
05/07/2020 11:00, Thomas Monjalon:
> 05/07/2020 10:51, Fady Bader:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 05/07/2020 09:00, Fady Bader:
> > > > > On 30/06/2020 11:49, Kinsella, Ray wrote:
> > > > > > On 22/06/2020 12:55, Fady Bader wrote:
> > > > > >> --- a/lib/librte_eal/include/rte_function_versioning.h
> > > > > >> +++ b/lib/librte_eal/include/rte_function_versioning.h
> > > > > >> @@ -11,6 +11,10 @@
> > > > > >>  #error Use of function versioning disabled, is
> > > "use_function_versioning=true"
> > > > > in meson.build?
> > > > > > Do we need a meson error/rule that catches if people explicitly
> > > > > > try to enable
> > > > > function_versioning.
> > > > > > We don't want to just silently fail under such circumstances?
> > > > > >
> > > > > > Would something in lib/meson.build would work better?
> > > > > >
> > > > > > if use_function_versioning and target_machine.system == "Windows"
> > > > > > 	// complain loudly ...
> > > > > > endif
> > > > > >
> > > >
> > > > I think this would be a better approach.
> > > > But instead of checking if the use of function versioning is enabled
> > > > under Windows and then complain, I think we should simply disable it
> > > > from Windows in the meson.build file. Something like this:
> > > >
> > > > if target_machine.system == "Windows"
> > > > 	use_function_versioning = false
> > > > endif
> > > 
> > > Did you try?
> > > 
> > > If you disable function versioning, compilation will fail with the #error message
> > > above, right?
> > >
> > 
> > Yes, the compilation fails, we can also change the rte_function_versioning.h
> > code in order not to fail under Windows.
> > What do you think ?
> 
> Function versioning CANNOT be supported currently on Windows.
> Function versioning macros are MANDATORY in some libraries.
> That's why I think the only path is to have a specific
> implementation of the function versioning macros for Windows.

After a closer look with Fady, the empty macros already exist
for the static case.
If disabling function versioning in shared case, the DLL is built
with the object files compiled for the static library.
Conclusion: disabling function versioning on Windows works
(after disabling the error message).
Ray Kinsella July 7, 2020, 7:57 a.m. UTC | #10 
On 05/07/2020 21:00, Thomas Monjalon wrote:
> 05/07/2020 11:00, Thomas Monjalon:
>> 05/07/2020 10:51, Fady Bader:
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> 05/07/2020 09:00, Fady Bader:
>>>>>> On 30/06/2020 11:49, Kinsella, Ray wrote:
>>>>>>> On 22/06/2020 12:55, Fady Bader wrote:
>>>>>>>> --- a/lib/librte_eal/include/rte_function_versioning.h
>>>>>>>> +++ b/lib/librte_eal/include/rte_function_versioning.h
>>>>>>>> @@ -11,6 +11,10 @@
>>>>>>>>  #error Use of function versioning disabled, is
>>>> "use_function_versioning=true"
>>>>>> in meson.build?
>>>>>>> Do we need a meson error/rule that catches if people explicitly
>>>>>>> try to enable
>>>>>> function_versioning.
>>>>>>> We don't want to just silently fail under such circumstances?
>>>>>>>
>>>>>>> Would something in lib/meson.build would work better?
>>>>>>>
>>>>>>> if use_function_versioning and target_machine.system == "Windows"
>>>>>>> 	// complain loudly ...
>>>>>>> endif
>>>>>>>
>>>>>
>>>>> I think this would be a better approach.
>>>>> But instead of checking if the use of function versioning is enabled
>>>>> under Windows and then complain, I think we should simply disable it
>>>>> from Windows in the meson.build file. Something like this:
>>>>>
>>>>> if target_machine.system == "Windows"
>>>>> 	use_function_versioning = false
>>>>> endif
>>>>
>>>> Did you try?
>>>>
>>>> If you disable function versioning, compilation will fail with the #error message
>>>> above, right?
>>>>
>>>
>>> Yes, the compilation fails, we can also change the rte_function_versioning.h
>>> code in order not to fail under Windows.
>>> What do you think ?
>>
>> Function versioning CANNOT be supported currently on Windows.
>> Function versioning macros are MANDATORY in some libraries.
>> That's why I think the only path is to have a specific
>> implementation of the function versioning macros for Windows.
> 
> After a closer look with Fady, the empty macros already exist
> for the static case.
> If disabling function versioning in shared case, the DLL is built
> with the object files compiled for the static library.
> Conclusion: disabling function versioning on Windows works
> (after disabling the error message).
> 

perfect, good work.
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_function_versioning.h b/lib/librte_eal/include/rte_function_versioning.h
index f588f2643b..cee06602e9 100644
--- a/lib/librte_eal/include/rte_function_versioning.h
+++ b/lib/librte_eal/include/rte_function_versioning.h
@@ -11,6 +11,10 @@ 
 #error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
 #endif
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#undef RTE_BUILD_SHARED_LIB
+#endif
+
 #ifdef RTE_BUILD_SHARED_LIB
 
 /*