[RFC,1/2] Add __rte_internal tag for functions and version target
diff mbox series

Message ID 20190525184346.27932-2-nhorman@tuxdriver.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • introduce __rte_internal tag
Related show

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Neil Horman May 25, 2019, 6:43 p.m. UTC
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(-)

Comments

Jerin Jacob Kollanukkaran June 5, 2019, 4:14 p.m. UTC | #1
> -----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?

Patch
diff mbox series

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$"
+	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
@@ -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")))
+#else
+#define __rte_internal __attribute__((error("This function cannot be used outside of the core DPDK library"), \
+	section(".text.internal")))
+#endif
 #endif /* _RTE_COMPAT_H_ */
diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
index 0cf3791b4..f1d97ef76 100644
--- a/mk/internal/rte.compile-pre.mk
+++ b/mk/internal/rte.compile-pre.mk
@@ -56,8 +56,8 @@  C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CPPFLAGS) $(CFLAGS) \
 C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight
 C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  CC $(@)")
 endif
-EXPERIMENTAL_CHECK = $(RTE_SDK)/buildtools/check-experimental-syms.sh
-CHECK_EXPERIMENTAL = $(EXPERIMENTAL_CHECK) $(SRCDIR)/$(EXPORT_MAP) $@
+SPECIAL_SYM_CHECK = $(RTE_SDK)/buildtools/check-special-syms.sh
+CHECK_SPECIAL_SYMS = $(SPECIAL_SYM_CHECK) $(SRCDIR)/$(EXPORT_MAP) $@
 
 PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
 PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) $(EXTRA_CFLAGS) -c -o $@.pmd.o $@.pmd.c
@@ -75,7 +75,7 @@  C_TO_O_DO = @set -e; \
 	echo $(C_TO_O_DISP); \
 	$(C_TO_O) && \
 	$(PMDINFO_TO_O) && \
-	$(CHECK_EXPERIMENTAL) && \
+	$(CHECK_SPECIAL_SYMS) && \
 	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
 	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
 	rm -f $(call obj2dep,$(@)).tmp
diff --git a/mk/target/generic/rte.vars.mk b/mk/target/generic/rte.vars.mk
index 25a578ad7..ed6a0c87b 100644
--- a/mk/target/generic/rte.vars.mk
+++ b/mk/target/generic/rte.vars.mk
@@ -96,7 +96,7 @@  LDFLAGS += -L$(RTE_OUTPUT)/lib
 # defined.
 ifeq ($(BUILDING_RTE_SDK),1)
 # building sdk
-CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
+CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h -DBUILDING_RTE_SDK
 else
 # if we are building an external application, include SDK's lib and
 # includes too