[v6,2/8] eal: fix error attribute use for clang
Checks
Commit Message
Clang does not have an "error" attribute for functions, so for marking
internal functions we need to check for the error attribute, and provide
a fallback if it is not present. For clang, we can use "diagnose_if"
attribute, similarly checking for its presence before use.
Fixes: fba5af82adc8 ("eal: add internal ABI tag definition")
Cc: haiyue.wang@intel.com
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
lib/librte_eal/include/rte_compat.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Comments
On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Clang does not have an "error" attribute for functions, so for marking
> internal functions we need to check for the error attribute, and provide
> a fallback if it is not present. For clang, we can use "diagnose_if"
> attribute, similarly checking for its presence before use.
>
> Fixes: fba5af82adc8 ("eal: add internal ABI tag definition")
> Cc: haiyue.wang@intel.com
Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> lib/librte_eal/include/rte_compat.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/include/rte_compat.h b/lib/librte_eal/include/rte_compat.h
> index 4cd8f68d68..c30f072aa3 100644
> --- a/lib/librte_eal/include/rte_compat.h
> +++ b/lib/librte_eal/include/rte_compat.h
> @@ -19,12 +19,18 @@ __attribute__((section(".text.experimental")))
>
> #endif
>
> -#ifndef ALLOW_INTERNAL_API
> +#if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */
>
> #define __rte_internal \
> __attribute__((error("Symbol is not public ABI"), \
> section(".text.internal")))
>
> +#elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */
> +
> +#define __rte_internal \
> +__attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> +section(".text.internal")))
> +
If the compiler has neither error or diagnose_if support, an internal
API can be called without ALLOW_INTERNAL_API.
I prefer a build error complaining on an unknown attribute rather than
silence a check.
I.e.
#ifndef ALLOW_INTERNAL_API
#if __has_attribute(diagnose_if) /* For clang */
#define __rte_internal \
__attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
section(".text.internal")))
#else
#define __rte_internal \
__attribute__((error("Symbol is not public ABI"), \
section(".text.internal")))
#endif
#else /* ALLOW_INTERNAL_API */
#define __rte_internal \
section(".text.internal")))
#endif
On Thu, Jan 28, 2021 at 12:00:46PM +0100, David Marchand wrote:
> On Wed, Jan 27, 2021 at 6:33 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > Clang does not have an "error" attribute for functions, so for marking
> > internal functions we need to check for the error attribute, and provide
> > a fallback if it is not present. For clang, we can use "diagnose_if"
> > attribute, similarly checking for its presence before use.
> >
> > Fixes: fba5af82adc8 ("eal: add internal ABI tag definition")
> > Cc: haiyue.wang@intel.com
>
> Cc: stable@dpdk.org
>
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > lib/librte_eal/include/rte_compat.h | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/include/rte_compat.h b/lib/librte_eal/include/rte_compat.h
> > index 4cd8f68d68..c30f072aa3 100644
> > --- a/lib/librte_eal/include/rte_compat.h
> > +++ b/lib/librte_eal/include/rte_compat.h
> > @@ -19,12 +19,18 @@ __attribute__((section(".text.experimental")))
> >
> > #endif
> >
> > -#ifndef ALLOW_INTERNAL_API
> > +#if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */
> >
> > #define __rte_internal \
> > __attribute__((error("Symbol is not public ABI"), \
> > section(".text.internal")))
> >
> > +#elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */
> > +
> > +#define __rte_internal \
> > +__attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > +section(".text.internal")))
> > +
>
> If the compiler has neither error or diagnose_if support, an internal
> API can be called without ALLOW_INTERNAL_API.
> I prefer a build error complaining on an unknown attribute rather than
> silence a check.
>
> I.e.
>
> #ifndef ALLOW_INTERNAL_API
>
> #if __has_attribute(diagnose_if) /* For clang */
> #define __rte_internal \
> __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> section(".text.internal")))
>
> #else
> #define __rte_internal \
> __attribute__((error("Symbol is not public ABI"), \
> section(".text.internal")))
>
> #endif
>
> #else /* ALLOW_INTERNAL_API */
>
> #define __rte_internal \
> section(".text.internal")))
>
> #endif
>
Would this not mean that if someone was using a compiler that supported
neither that they could never include any header file which contained any
internal functions? I'd rather err on the side of allowing this, on the
basis that the symbol should be already documented as internal and this is
only an additional sanity check.
/Bruce
On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > If the compiler has neither error or diagnose_if support, an internal
> > API can be called without ALLOW_INTERNAL_API.
> > I prefer a build error complaining on an unknown attribute rather than
> > silence a check.
> >
> > I.e.
> >
> > #ifndef ALLOW_INTERNAL_API
> >
> > #if __has_attribute(diagnose_if) /* For clang */
> > #define __rte_internal \
> > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > section(".text.internal")))
> >
> > #else
> > #define __rte_internal \
> > __attribute__((error("Symbol is not public ABI"), \
> > section(".text.internal")))
> >
> > #endif
> >
> > #else /* ALLOW_INTERNAL_API */
> >
> > #define __rte_internal \
> > section(".text.internal")))
> >
> > #endif
> >
>
> Would this not mean that if someone was using a compiler that supported
> neither that they could never include any header file which contained any
> internal functions? I'd rather err on the side of allowing this, on the
> basis that the symbol should be already documented as internal and this is
> only an additional sanity check.
- Still not a fan.
We will never know about those compilers behavior, like how we caught
the clang issue and found an alternative.
- I just caught a build error with RHEL7 gcc:
[1/2127] Compiling C object
lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o
FAILED: lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o
ccache cc -Ilib/librte_eal.a.p -Ilib -I../lib -I. -I.. -Iconfig
-I../config -Ilib/librte_eal/include -I../lib/librte_eal/include
-Ilib/librte_eal/linux/include -I../lib/librte_eal/linux/include
-Ilib/librte_eal/x86/include -I../lib/librte_eal/x86/include
-Ilib/librte_eal/common -I../lib/librte_eal/common -Ilib/librte_eal
-I../lib/librte_eal -Ilib/librte_kvargs -I../lib/librte_kvargs
-Ilib/librte_metrics -I../lib/librte_metrics -Ilib/librte_telemetry
-I../lib/librte_telemetry -pipe -D_FILE_OFFSET_BITS=64 -Wall
-Winvalid-pch -O3 -include rte_config.h -Wextra -Wcast-qual
-Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
-Wmissing-declarations -Wmissing-prototypes -Wnested-externs
-Wold-style-definition -Wpointer-arith -Wsign-compare
-Wstrict-prototypes -Wundef -Wwrite-strings
-Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=native
-DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API '-DABI_VERSION="21.1"'
-MD -MQ lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -MF
lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o.d -o
lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -c
../lib/librte_eal/common/eal_common_config.c
In file included from ../lib/librte_eal/include/rte_dev.h:24:0,
from ../lib/librte_eal/common/eal_private.h:12,
from ../lib/librte_eal/common/eal_common_config.c:9:
../lib/librte_eal/include/rte_compat.h:22:51: error: missing binary
operator before token "("
#if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */
^
../lib/librte_eal/include/rte_compat.h:28:53: error: missing binary
operator before token "("
#elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /*
For clang */
^
I can see that gcc doc recommends checking for __has_attribute availability.
Pasting from google cache, since the gcc.gnu.org doc website seems unavailable.
"""
4.2.6 __has_attribute
The special operator __has_attribute (operand) may be used in ‘#if’
and ‘#elif’ expressions to test whether the attribute referenced by
its operand is recognized by GCC. Using the operator in other contexts
is not valid. In C code, if compiling for strict conformance to
standards before C2x, operand must be a valid identifier. Otherwise,
operand may be optionally introduced by the attribute-scope:: prefix.
The attribute-scope prefix identifies the “namespace” within which the
attribute is recognized. The scope of GCC attributes is ‘gnu’ or
‘__gnu__’. The __has_attribute operator by itself, without any operand
or parentheses, acts as a predefined macro so that support for it can
be tested in portable code. Thus, the recommended use of the operator
is as follows:
#if defined __has_attribute
# if __has_attribute (nonnull)
# define ATTR_NONNULL __attribute__ ((nonnull))
# endif
#endif
The first ‘#if’ test succeeds only when the operator is supported by
the version of GCC (or another compiler) being used. Only when that
test succeeds is it valid to use __has_attribute as a preprocessor
operator. As a result, combining the two tests into a single
expression as shown below would only be valid with a compiler that
supports the operator but not with others that don’t.
#if defined __has_attribute && __has_attribute (nonnull) /* not portable */
…
#endif
"""
On Thu, Jan 28, 2021 at 02:36:25PM +0100, David Marchand wrote:
> On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > If the compiler has neither error or diagnose_if support, an internal
> > > API can be called without ALLOW_INTERNAL_API.
> > > I prefer a build error complaining on an unknown attribute rather than
> > > silence a check.
> > >
> > > I.e.
> > >
> > > #ifndef ALLOW_INTERNAL_API
> > >
> > > #if __has_attribute(diagnose_if) /* For clang */
> > > #define __rte_internal \
> > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > > section(".text.internal")))
> > >
> > > #else
> > > #define __rte_internal \
> > > __attribute__((error("Symbol is not public ABI"), \
> > > section(".text.internal")))
> > >
> > > #endif
> > >
> > > #else /* ALLOW_INTERNAL_API */
> > >
> > > #define __rte_internal \
> > > section(".text.internal")))
> > >
> > > #endif
> > >
> >
> > Would this not mean that if someone was using a compiler that supported
> > neither that they could never include any header file which contained any
> > internal functions? I'd rather err on the side of allowing this, on the
> > basis that the symbol should be already documented as internal and this is
> > only an additional sanity check.
>
> - Still not a fan.
> We will never know about those compilers behavior, like how we caught
> the clang issue and found an alternative.
>
So I understand, but I'm still concerned about breaking something that was
previously working. It's one thing a DPDK developer catching issues with
clang, quite another a user catching it when trying to build their own
application.
We probably need some other opinions on this one.
>
> - I just caught a build error with RHEL7 gcc:
>
> [1/2127] Compiling C object
> lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o
> FAILED: lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o
> ccache cc -Ilib/librte_eal.a.p -Ilib -I../lib -I. -I.. -Iconfig
> -I../config -Ilib/librte_eal/include -I../lib/librte_eal/include
> -Ilib/librte_eal/linux/include -I../lib/librte_eal/linux/include
> -Ilib/librte_eal/x86/include -I../lib/librte_eal/x86/include
> -Ilib/librte_eal/common -I../lib/librte_eal/common -Ilib/librte_eal
> -I../lib/librte_eal -Ilib/librte_kvargs -I../lib/librte_kvargs
> -Ilib/librte_metrics -I../lib/librte_metrics -Ilib/librte_telemetry
> -I../lib/librte_telemetry -pipe -D_FILE_OFFSET_BITS=64 -Wall
> -Winvalid-pch -O3 -include rte_config.h -Wextra -Wcast-qual
> -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
> -Wmissing-declarations -Wmissing-prototypes -Wnested-externs
> -Wold-style-definition -Wpointer-arith -Wsign-compare
> -Wstrict-prototypes -Wundef -Wwrite-strings
> -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=native
> -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API '-DABI_VERSION="21.1"'
> -MD -MQ lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -MF
> lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o.d -o
> lib/librte_eal.a.p/librte_eal_common_eal_common_config.c.o -c
> ../lib/librte_eal/common/eal_common_config.c
> In file included from ../lib/librte_eal/include/rte_dev.h:24:0,
> from ../lib/librte_eal/common/eal_private.h:12,
> from ../lib/librte_eal/common/eal_common_config.c:9:
> ../lib/librte_eal/include/rte_compat.h:22:51: error: missing binary
> operator before token "("
> #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */
> ^
> ../lib/librte_eal/include/rte_compat.h:28:53: error: missing binary
> operator before token "("
> #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /*
> For clang */
> ^
>
> I can see that gcc doc recommends checking for __has_attribute availability.
> Pasting from google cache, since the gcc.gnu.org doc website seems unavailable.
>
> """
> 4.2.6 __has_attribute
>
> The special operator __has_attribute (operand) may be used in ‘#if’
> and ‘#elif’ expressions to test whether the attribute referenced by
> its operand is recognized by GCC. Using the operator in other contexts
> is not valid. In C code, if compiling for strict conformance to
> standards before C2x, operand must be a valid identifier. Otherwise,
> operand may be optionally introduced by the attribute-scope:: prefix.
> The attribute-scope prefix identifies the “namespace” within which the
> attribute is recognized. The scope of GCC attributes is ‘gnu’ or
> ‘__gnu__’. The __has_attribute operator by itself, without any operand
> or parentheses, acts as a predefined macro so that support for it can
> be tested in portable code. Thus, the recommended use of the operator
> is as follows:
>
> #if defined __has_attribute
> # if __has_attribute (nonnull)
> # define ATTR_NONNULL __attribute__ ((nonnull))
> # endif
> #endif
>
> The first ‘#if’ test succeeds only when the operator is supported by
> the version of GCC (or another compiler) being used. Only when that
> test succeeds is it valid to use __has_attribute as a preprocessor
> operator. As a result, combining the two tests into a single
> expression as shown below would only be valid with a compiler that
> supports the operator but not with others that don’t.
>
> #if defined __has_attribute && __has_attribute (nonnull) /* not portable */
> …
> #endif
> """
>
I really wish other tools would do like meson and document per-feature the
version in which it was introduced! Anyway, this is something I'll fix in
next version, though again we need to decide in the case of __has_attribute
not being supported do we fall to erroring out? Again that runs the risk of
users not being able to include a header which has an internal function, so
I'd prefer us to ignore errors if the appropriate macros are unsupported.
Again, other opinions probably needed.
/Bruce
On Thu, Jan 28, 2021 at 02:16:10PM +0000, Bruce Richardson wrote:
> On Thu, Jan 28, 2021 at 02:36:25PM +0100, David Marchand wrote:
> > On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > > If the compiler has neither error or diagnose_if support, an internal
> > > > API can be called without ALLOW_INTERNAL_API.
> > > > I prefer a build error complaining on an unknown attribute rather than
> > > > silence a check.
> > > >
> > > > I.e.
> > > >
> > > > #ifndef ALLOW_INTERNAL_API
> > > >
> > > > #if __has_attribute(diagnose_if) /* For clang */
> > > > #define __rte_internal \
> > > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > > > section(".text.internal")))
> > > >
> > > > #else
> > > > #define __rte_internal \
> > > > __attribute__((error("Symbol is not public ABI"), \
> > > > section(".text.internal")))
> > > >
> > > > #endif
> > > >
> > > > #else /* ALLOW_INTERNAL_API */
> > > >
> > > > #define __rte_internal \
> > > > section(".text.internal")))
> > > >
> > > > #endif
> > > >
> > >
> > > Would this not mean that if someone was using a compiler that supported
> > > neither that they could never include any header file which contained any
> > > internal functions? I'd rather err on the side of allowing this, on the
> > > basis that the symbol should be already documented as internal and this is
> > > only an additional sanity check.
> >
> > - Still not a fan.
> > We will never know about those compilers behavior, like how we caught
> > the clang issue and found an alternative.
> >
>
> So I understand, but I'm still concerned about breaking something that was
> previously working. It's one thing a DPDK developer catching issues with
> clang, quite another a user catching it when trying to build their own
> application.
>
> We probably need some other opinions on this one.
>
Adding Tech-board to see if we can get some more thoughts on this before I do
another revision of this set.
28/01/2021 16:16, Bruce Richardson:
> On Thu, Jan 28, 2021 at 02:16:10PM +0000, Bruce Richardson wrote:
> > On Thu, Jan 28, 2021 at 02:36:25PM +0100, David Marchand wrote:
> > > On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > > If the compiler has neither error or diagnose_if support, an internal
> > > > > API can be called without ALLOW_INTERNAL_API.
> > > > > I prefer a build error complaining on an unknown attribute rather than
> > > > > silence a check.
> > > > >
> > > > > I.e.
> > > > >
> > > > > #ifndef ALLOW_INTERNAL_API
> > > > >
> > > > > #if __has_attribute(diagnose_if) /* For clang */
> > > > > #define __rte_internal \
> > > > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > > > > section(".text.internal")))
> > > > >
> > > > > #else
> > > > > #define __rte_internal \
> > > > > __attribute__((error("Symbol is not public ABI"), \
> > > > > section(".text.internal")))
> > > > >
> > > > > #endif
> > > > >
> > > > > #else /* ALLOW_INTERNAL_API */
> > > > >
> > > > > #define __rte_internal \
> > > > > section(".text.internal")))
> > > > >
> > > > > #endif
> > > > >
> > > >
> > > > Would this not mean that if someone was using a compiler that supported
> > > > neither that they could never include any header file which contained any
> > > > internal functions? I'd rather err on the side of allowing this, on the
> > > > basis that the symbol should be already documented as internal and this is
> > > > only an additional sanity check.
> > >
> > > - Still not a fan.
> > > We will never know about those compilers behavior, like how we caught
> > > the clang issue and found an alternative.
> > >
> >
> > So I understand, but I'm still concerned about breaking something that was
> > previously working. It's one thing a DPDK developer catching issues with
> > clang, quite another a user catching it when trying to build their own
> > application.
> >
> > We probably need some other opinions on this one.
> >
> Adding Tech-board to see if we can get some more thoughts on this before I do
> another revision of this set.
What are the alternatives?
On Thu, Jan 28, 2021 at 05:46:16PM +0100, Thomas Monjalon wrote:
> 28/01/2021 16:16, Bruce Richardson:
> > On Thu, Jan 28, 2021 at 02:16:10PM +0000, Bruce Richardson wrote:
> > > On Thu, Jan 28, 2021 at 02:36:25PM +0100, David Marchand wrote:
> > > > On Thu, Jan 28, 2021 at 12:20 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > > If the compiler has neither error or diagnose_if support, an internal
> > > > > > API can be called without ALLOW_INTERNAL_API.
> > > > > > I prefer a build error complaining on an unknown attribute rather than
> > > > > > silence a check.
> > > > > >
> > > > > > I.e.
> > > > > >
> > > > > > #ifndef ALLOW_INTERNAL_API
> > > > > >
> > > > > > #if __has_attribute(diagnose_if) /* For clang */
> > > > > > #define __rte_internal \
> > > > > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
> > > > > > section(".text.internal")))
> > > > > >
> > > > > > #else
> > > > > > #define __rte_internal \
> > > > > > __attribute__((error("Symbol is not public ABI"), \
> > > > > > section(".text.internal")))
> > > > > >
> > > > > > #endif
> > > > > >
> > > > > > #else /* ALLOW_INTERNAL_API */
> > > > > >
> > > > > > #define __rte_internal \
> > > > > > section(".text.internal")))
> > > > > >
> > > > > > #endif
> > > > > >
> > > > >
> > > > > Would this not mean that if someone was using a compiler that supported
> > > > > neither that they could never include any header file which contained any
> > > > > internal functions? I'd rather err on the side of allowing this, on the
> > > > > basis that the symbol should be already documented as internal and this is
> > > > > only an additional sanity check.
> > > >
> > > > - Still not a fan.
> > > > We will never know about those compilers behavior, like how we caught
> > > > the clang issue and found an alternative.
> > > >
> > >
> > > So I understand, but I'm still concerned about breaking something that was
> > > previously working. It's one thing a DPDK developer catching issues with
> > > clang, quite another a user catching it when trying to build their own
> > > application.
> > >
> > > We probably need some other opinions on this one.
> > >
> > Adding Tech-board to see if we can get some more thoughts on this before I do
> > another revision of this set.
>
> What are the alternatives?
>
The basic problem is what to do when a compiler is used which does not support
the required macros to flag an error for use of an internal-only function.
For example, we discovered this because clang does not support the #error
macro.
In those cases, as I see it, we really have two choices:
1 ignore flagging the error and silently allow possible use of the internal
function.
2 have the compiler flag an error for an unknown macro
The problem that I have with #2 is that without knowing the macro, the
compiler will likely error out any time a user app includes any header with
an internal function, even if the function is unused.
On the other hand, the likelihood of impact if we choose #2 and do error out is
quite small, since modern clang versions will support the modern macro
checks we need, and just about any gcc versions we care about are going to
support #error.
For #1, the downside is that we will miss error checks on some older
versions of gcc e.g. RHEL 7, and the user may inadvertently use an internal
function without knowing it.
David, anything else to add here?
/Bruce
On Thu, Jan 28, 2021 at 6:36 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > > > We will never know about those compilers behavior, like how we caught
> > > > > the clang issue and found an alternative.
> > > > >
> > > >
> > > > So I understand, but I'm still concerned about breaking something that was
> > > > previously working. It's one thing a DPDK developer catching issues with
> > > > clang, quite another a user catching it when trying to build their own
> > > > application.
> > > >
> > > > We probably need some other opinions on this one.
> > > >
> > > Adding Tech-board to see if we can get some more thoughts on this before I do
> > > another revision of this set.
> >
> > What are the alternatives?
> >
>
> The basic problem is what to do when a compiler is used which does not support
> the required macros to flag an error for use of an internal-only function.
> For example, we discovered this because clang does not support the #error
> macro.
*attribute*
>
> In those cases, as I see it, we really have two choices:
>
> 1 ignore flagging the error and silently allow possible use of the internal
> function.
> 2 have the compiler flag an error for an unknown macro
>
> The problem that I have with #2 is that without knowing the macro, the
> compiler will likely error out any time a user app includes any header with
> an internal function, even if the function is unused.
>
> On the other hand, the likelihood of impact if we choose #2 and do error out is
> quite small, since modern clang versions will support the modern macro
> checks we need, and just about any gcc versions we care about are going to
> support #error.
>
> For #1, the downside is that we will miss error checks on some older
> versions of gcc e.g. RHEL 7, and the user may inadvertently use an internal
> function without knowing it.
>
> David, anything else to add here?
Nop, fair summary.
29/01/2021 09:35, David Marchand:
> On Thu, Jan 28, 2021 at 6:36 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > > > We will never know about those compilers behavior, like how we caught
> > > > > > the clang issue and found an alternative.
> > > > > >
> > > > >
> > > > > So I understand, but I'm still concerned about breaking something that was
> > > > > previously working. It's one thing a DPDK developer catching issues with
> > > > > clang, quite another a user catching it when trying to build their own
> > > > > application.
> > > > >
> > > > > We probably need some other opinions on this one.
> > > > >
> > > > Adding Tech-board to see if we can get some more thoughts on this before I do
> > > > another revision of this set.
> > >
> > > What are the alternatives?
> > >
> >
> > The basic problem is what to do when a compiler is used which does not support
> > the required macros to flag an error for use of an internal-only function.
> > For example, we discovered this because clang does not support the #error
> > macro.
>
> *attribute*
>
> >
> > In those cases, as I see it, we really have two choices:
> >
> > 1 ignore flagging the error and silently allow possible use of the internal
> > function.
> > 2 have the compiler flag an error for an unknown macro
> >
> > The problem that I have with #2 is that without knowing the macro, the
> > compiler will likely error out any time a user app includes any header with
> > an internal function, even if the function is unused.
> >
> > On the other hand, the likelihood of impact if we choose #2 and do error out is
> > quite small, since modern clang versions will support the modern macro
> > checks we need, and just about any gcc versions we care about are going to
> > support #error.
If there are very small chances of hitting the issue, it looks acceptable.
But I don't really like risking issues with not tested compilers.
> > For #1, the downside is that we will miss error checks on some older
> > versions of gcc e.g. RHEL 7, and the user may inadvertently use an internal
> > function without knowing it.
The function is marked as internal.
If the user misses this info and test only with compilers not supporting
the attribute, that's no luck.
What would be the consequence? Having a risk for future compatibility?
It looks to be an acceptable risk.
The other consequence it that we won't have reports for this rare
incompatibility.
> > David, anything else to add here?
>
> Nop, fair summary.
My preference is for #1:
pro: No compilation will be impacted in the field
con: DPDK developers may miss some cases
Any other opinion?
@@ -19,12 +19,18 @@ __attribute__((section(".text.experimental")))
#endif
-#ifndef ALLOW_INTERNAL_API
+#if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */
#define __rte_internal \
__attribute__((error("Symbol is not public ABI"), \
section(".text.internal")))
+#elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */
+
+#define __rte_internal \
+__attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \
+section(".text.internal")))
+
#else
#define __rte_internal \