[v2,01/10] Add __rte_internal tag for functions and version target
Checks
Commit Message
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>
---
...rimental-syms.sh => check-special-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(-)
rename buildtools/{check-experimental-syms.sh => check-special-syms.sh} (53%)
Comments
Hi Neil,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Neil Horman
> Sent: Thursday, June 13, 2019 22:24
> To: dev@dpdk.org
> Cc: Neil Horman <nhorman@tuxdriver.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Subject: [dpdk-dev] [PATCH v2 01/10] 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>
> ---
> ...rimental-syms.sh => check-special-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(-)
> rename buildtools/{check-experimental-syms.sh => check-special-syms.sh} (53%)
>
....
> 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_ */
Since struct definition is also a kind of ABI (am I right ? ;-) ), like:
drivers/bus/pci/rte_bus_pci.h
struct rte_pci_device {
...
struct rte_intr_handle vfio_req_intr_handle;
/**< Handler of VFIO request interrupt */
} __rte_internal;
Then will capture the errors anyway by using one of __rte_internal definition.
error: 'section' attribute does not apply to types [-Werror=attributes]
error: 'error' attribute does not apply to types
> 2.20.1
On Fri, Apr 17, 2020 at 02:04:30AM +0000, Wang, Haiyue wrote:
> Hi Neil,
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Neil Horman
> > Sent: Thursday, June 13, 2019 22:24
> > To: dev@dpdk.org
> > Cc: Neil Horman <nhorman@tuxdriver.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Richardson,
> > Bruce <bruce.richardson@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> > Subject: [dpdk-dev] [PATCH v2 01/10] 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>
> > ---
> > ...rimental-syms.sh => check-special-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(-)
> > rename buildtools/{check-experimental-syms.sh => check-special-syms.sh} (53%)
> >
> ....
>
> > 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_ */
>
> Since struct definition is also a kind of ABI (am I right ? ;-) ), like:
>
Yes, thats correct, which is why I've advocated for making structs
opaque as part of the abi, but I suppose thats not where we are. :)
> drivers/bus/pci/rte_bus_pci.h
> struct rte_pci_device {
> ...
> struct rte_intr_handle vfio_req_intr_handle;
> /**< Handler of VFIO request interrupt */
> } __rte_internal;
>
> Then will capture the errors anyway by using one of __rte_internal definition.
> error: 'section' attribute does not apply to types [-Werror=attributes]
> error: 'error' attribute does not apply to types
>
As it is currently written, the __rte_internal macro is only written to
work on functions. If you don't want a struct to be part of the ABI, we
would need to either:
a) make a simmilar macro (say __rte_internal_data) which uses a simmilar
gcc attibute to catch external usage.
or
b) just move the strucute definition to a location that isn't exposed as
part of the external ABI
Neil
> > 2.20.1
>
>
> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Friday, April 17, 2020 10:38
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Thomas Monjalon <thomas@monjalon.net>; David Marchand
> <david.marchand@redhat.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 01/10] Add __rte_internal tag for functions and version target
>
> On Fri, Apr 17, 2020 at 02:04:30AM +0000, Wang, Haiyue wrote:
> > Hi Neil,
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Neil Horman
> > > Sent: Thursday, June 13, 2019 22:24
> > > To: dev@dpdk.org
> > > Cc: Neil Horman <nhorman@tuxdriver.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Richardson,
> > > Bruce <bruce.richardson@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> > > Subject: [dpdk-dev] [PATCH v2 01/10] 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>
> > > ---
> > > ...rimental-syms.sh => check-special-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(-)
> > > rename buildtools/{check-experimental-syms.sh => check-special-syms.sh} (53%)
> > >
> > ....
> >
> > > 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_ */
> >
> > Since struct definition is also a kind of ABI (am I right ? ;-) ), like:
> >
> Yes, thats correct, which is why I've advocated for making structs
> opaque as part of the abi, but I suppose thats not where we are. :)
>
Make sense, normally structs can't live alone without function API. A little
paranoid for type only ABI checking. ;-) And this definition is good for ABI
checking as we did it for EXPERIMENTAL.
Thanks!
Haiyue
> > drivers/bus/pci/rte_bus_pci.h
> > struct rte_pci_device {
> > ...
> > struct rte_intr_handle vfio_req_intr_handle;
> > /**< Handler of VFIO request interrupt */
> > } __rte_internal;
> >
> > Then will capture the errors anyway by using one of __rte_internal definition.
> > error: 'section' attribute does not apply to types [-Werror=attributes]
> > error: 'error' attribute does not apply to types
> >
> As it is currently written, the __rte_internal macro is only written to
> work on functions. If you don't want a struct to be part of the ABI, we
> would need to either:
>
> a) make a simmilar macro (say __rte_internal_data) which uses a simmilar
> gcc attibute to catch external usage.
>
> or
>
> b) just move the strucute definition to a location that isn't exposed as
> part of the external ABI
>
> Neil
>
> > > 2.20.1
> >
> >
similarity index 53%
rename from buildtools/check-experimental-syms.sh
rename to buildtools/check-special-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
@@ -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_ */
@@ -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
@@ -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