Re: [dpdk-dev] [PATCH v2 01/10] build: add an option to enable LTO build

[Thomas Monjalon <thomas@monjalon.net>]

28/10/2019 09:36, Andrzej Ostruszka:
> On 10/27/19 12:31 PM, Thomas Monjalon wrote:
> > 18/09/2019 15:32, Ray Kinsella:
> >> this is cool, good work.
> >> comments below.
> >>
> >> On 17/09/2019 08:57, Andrzej Ostruszka wrote:
> >>> --- a/lib/librte_distributor/rte_distributor.c
> >>> +++ b/lib/librte_distributor/rte_distributor.c
> >>> @@ -32,7 +32,7 @@ EAL_REGISTER_TAILQ(rte_dist_burst_tailq)
> >>>  
> >>>  /**** Burst Packet APIs called by workers ****/
> >>>  
> >>> -void
> >>> +void __vsym
> >>
> >> all these additional __vsym annotations looks like they belong in a
> >> seperate patch, as they are fixing a bug and are not directly related to
> >> adding LTO the build system.
> > 
> > Andrzej, you did not reply to this question.
> > This is a real blocker for merging this series.
> 
> Thomas, thank you for the reminder.  Somehow that comment has escaped me
> - although I've read it then.
> 
> > Should __vsym addition be in a separate patch?
> 
> I'm fine both ways.  You could argue that:
> - it is a bug since '__vsym' clearly annotates the function as being
>   used as a particular version of a symbol and as such it was missing
> - or as a part of enablement for LTO since without it compiler/linker
>   should not be removing given function and '__vsym' really is just a
>   "attribute(used)" to tell optimizing compiler/linker that this
>   function should not be removed.
> 
> Since you raised that question I'm guessing that you prefer it to be in
> a separate patch - so, unless you object now, I'm going to split it in
> the next version and have it as a first patch.
> 
> > Should we document its use in rte_function_versioning.h
> > and versioning.rst?
> 
> Yes, I think so.  I'll add that.
> 
> Again, thank you for the reminder and comments.

Thank you, a separate patch with doc update is very welcome.