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. Regards Andrzej
