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

[Bruce Richardson]

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 at 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