Re: [dpdk-dev] [EXT] [RFC PATCH 1/2] Add __rte_internal tag for functions and version target

[Jerin Jacob Kollanukkaran <jerinj@marvell.com>]

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Sunday, May 26, 2019 12:14 AM
> To: dev@dpdk.org
> Cc: Neil Horman <nhorman@tuxdriver.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Subject: [EXT] [RFC PATCH 1/2] Add __rte_internal tag for functions and
> version target
> This tag is meant to be used on function prototypes to identify functions that
> are only meant to be used by internal DPDK libraries (i.e. libraries that are
> built while building the SDK itself, as identified by the defining of the
> BUILDING_RTE_SDK macro).  When that flag is not set, it will resolve to an
> error function attribute, causing build breakage for any compilation unit
> attempting to build it
> 
> Validate the use of this tag in much the same way we validate
> __rte_experimental.  By adding an INTERNAL version to library map files, we
> can exempt internal-only functions from ABI checking, and handle them to
> ensure that symbols we wish to only be for internal use between dpdk
> libraries are properly tagged with __rte_experimental
> 
> Note this patch updates the check-experimental-syms.sh script, which
> normally only check the EXPERIMENTAL section to also check the INTERNAL
> section now.  As such its been renamed to the now more appropriate check-
> special-syms.sh
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> CC: Bruce Richardson <bruce.richardson@intel.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
> ---
>  buildtools/check-experimental-syms.sh      | 24 +++++++++++++++++++++-
>  lib/librte_eal/common/include/rte_compat.h | 12 +++++++++++
>  mk/internal/rte.compile-pre.mk             |  6 +++---
>  mk/target/generic/rte.vars.mk              |  2 +-
>  4 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-
> experimental-syms.sh
> index 7d1f3a568..63682c677 100755
> --- a/buildtools/check-experimental-syms.sh
> +++ b/buildtools/check-experimental-syms.sh
> @@ -31,10 +31,32 @@ do
>  		cat >&2 <<- END_OF_MESSAGE
>  		$SYM is not flagged as experimental
>  		but is listed in version map
> -		Please add __rte_experimental to the definition of $SYM
> +		Please add __rte_experimental to the definition/prototype
> of $SYM
>  		END_OF_MESSAGE
>  		exit 1
>  	fi
>  done
> +
> +for i in `awk 'BEGIN {found=0}
> +		/.*INTERNAL.*/ {found=1}
> +		/.*}.*;/ {found=0}
> +		/.*;/ {if (found == 1) print $1}' $MAPFILE` do
> +	SYM=`echo $i | sed -e"s/;//"`
> +	objdump -t $OBJFILE | grep -q "\.text.*$SYM$"
> +	IN_TEXT=$?
> +	objdump -t $OBJFILE | grep -q "\.text\.internal.*$SYM$"

I think, it should be OK for cross compilation case. But please cross check.

> +	IN_EXP=$?
> +	if [ $IN_TEXT -eq 0 -a $IN_EXP -ne 0 ]
> +	then
> +		cat >&2 <<- END_OF_MESSAGE
> +		$SYM is not flagged as internal
> +		but is listed in version map
> +		Please add __rte_internal to the definition/prototype of
> $SYM
> +		END_OF_MESSAGE
> +		exit 1
> +	fi
> +done
> +
>  exit 0
> 
> diff --git a/lib/librte_eal/common/include/rte_compat.h
> b/lib/librte_eal/common/include/rte_compat.h
> index 92ff28faf..739e8485c 100644
> --- a/lib/librte_eal/common/include/rte_compat.h
> +++ b/lib/librte_eal/common/include/rte_compat.h


It is a public header file, How about, having __rte_internal
In internal header file so that only SDK components can use it.

> @@ -89,4 +89,16 @@ __attribute__((section(".text.experimental")))
> 
>  #endif
> 
> +/*
> + * __rte_internal tags mark functions as internal only, If specified in
> +public
> + * header files, this tag will resolve to an error directive,
> +preventing
> + * external applications from attempting to make calls to functions not
> +meant
> + * for consumption outside the dpdk library  */ #ifdef BUILDING_RTE_SDK
> +#define __rte_internal __attribute__((section(".text.internal")))


Sometimes, we may have global variables like variable for dynamic logging etc.
If so, I think, it is better to change something not starting with .text


> +#else
> +#define __rte_internal __attribute__((error("This function cannot be used
> outside of the core DPDK library"), \
> +	section(".text.internal")))

Since the above statement is an error, Do we need section attribute here?