[dpdk-dev] [PATCH v6 2/8] eal: fix error attribute use for clang

[Bruce Richardson]

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 at 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 at intel.com
> 
> Cc: stable at dpdk.org
> 
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson at 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