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.richard...@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
