> > +#ifndef ODP_ABI_EVENT_H_
> > +#define ODP_ABI_EVENT_H_
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <stdint.h>
> > +
> > +/** @internal Dummy type for strong typing */
> > +typedef struct { char dummy; /**< @internal Dummy */ }
> _odp_abi_event_t;
>
> Why change from uint8_t to char here? We settled on uint8_t because
> that has a precise size definition while char may be 2 bytes on some
> systems and 1 byte on others. Granted this is never referenced, so
> it's benign, however ABI checkers might flag this as a potential
> difference, so I think it's safer and more portable to stick with
> uint8_t here.
>
> Also, since these internal typedefs are not referenced outside of this
> routine, why include them at all? Previously we wrappered this in the
> ODP_HANDLE_T internal macro. That could be renamed to something to
> avoid the ODP prefix but having this common scaffolding in one place
> (odp_strong_types.h in the original) instead of replicated in each
> file would seem to make for easier maintenance.
The dummy type should not matter since it's never accessed. It's probably vice
to have it smallest addressable type aligned (== char).
Within one ABI spec (in general) each type must be well defined. An ABI defined
char either 1 or 2 bytes (not both), so uint8_t is not needed. Application is
recompiled between two different ABIs.
> > +#include <odp/arch/default/api/abi/event.h>
> > diff --git a/platform/Makefile.inc b/platform/Makefile.inc
> > index 2722946..a24accb 100644
> > --- a/platform/Makefile.inc
> > +++ b/platform/Makefile.inc
> > @@ -60,6 +60,14 @@ odpapispecinclude_HEADERS = \
> > $(top_builddir)/include/odp/api/spec/version.h \
> > $(top_srcdir)/include/odp/api/spec/traffic_mngr.h
> >
> > +odpapiabidefaultincludedir= $(includedir)/odp/arch/default/api/abi
> > +odpapiabidefaultinclude_HEADERS = \
> > + $(top_srcdir)/include/odp/arch/default/api/abi/event.h
>
> Some judicious use of underscores would make these long names a lot
> more readable. e.g., odp_api_abi_default_include_dir = ...
This is our current naming convention on Makefiles. The previous line does not
show here, but it's the same format.
> > diff --git a/platform/linux-generic/include/odp/api/plat/event_types.h
> b/platform/linux-generic/include/odp/api/plat/event_types.h
> > index 9ca0fb8..a1aa0e4 100644
> > --- a/platform/linux-generic/include/odp/api/plat/event_types.h
> > +++ b/platform/linux-generic/include/odp/api/plat/event_types.h
> > @@ -18,11 +18,15 @@
> > extern "C" {
> > #endif
> >
> > +#include <odp/api/plat/static_inline.h>
> > +#if ODP_ABI_COMPAT == 1
> > +#include <odp/api/abi/event.h>
> > +#else
> > +
> > #include <odp/api/std_types.h>
> > #include <odp/api/plat/strong_types.h>
>
> Strong typing is a significant plus for helping to avoid programming
> errors, but it's really orthogonal to the question of whether ABI
> compatibility is being used, so it's strange to see all of this
> duplication in each of the type definition files.
>
> Since handles are opaque objects, from an ABI perspective, the only
> requirement on handles is that they be of the same size and have the
> same alignment across ABI-compatible implementations, which is what
> we've already done. Strong typing just means that these opaque objects
> come in different colors so that they can be distinguished at compile
> time.
>
> I'm not sure if we want to encourage or support non-strongly typed ODP
> implementations, and certainly not within odp-linux, but I don't think
> tying this to ABI compatibility is a good idea. Given the use of
> opaque handles, the main ABI difference is in the use of inlined
> functions, because these break handle opacity.
>
#include <odp/api/abi/event.h>
This line says: "Use ODP defined ABI spec, whatever that might be"
Everything else is (currently) left as it is. We can clean out odp-linux
implementation as soon as ABI spec content is 100% fixed. Also ABI spec could
be different between architectures, not on everything but on some small
details. E.g. x86 implementations are few and likely DPDK based, e.g. those
could decide to map handles directly to DPDK types. Whereas a bunch of ARM
vendors could agree on something different on ARMv8 ABI.
Type sizes may very well be different between ABIs. We may force strong typing
in ABI spec as I'm doing in this v2. Inline functions can be part of ABI if all
parties agree on the content (the exact code and data included).
> > index cc3fb0a..d71f446 100644
> > --- a/platform/linux-generic/odp_event.c
> > +++ b/platform/linux-generic/odp_event.c
> > @@ -38,3 +38,8 @@ void odp_event_free(odp_event_t event)
> > ODP_ABORT("Invalid event type: %d\n",
> odp_event_type(event));
> > }
> > }
> > +
> > +uint64_t odp_event_to_u64(odp_event_t hdl)
> > +{
> > + return _odp_pri(hdl);
> > +}
>
> Making this function non-inlined is OK from a performance perspective
> since it's not really a performance path, however this is an example
> of where an inlined function is ABI compatible because it doesn't
> really look inside handles. This is just a cast function that makes
> the object printable, but since we do not specify what sort of content
> handles contain, inlining here is no more non-ABI compatible than
> equality tests that exist in open code whenever applications compare
> two handles.
The ABI spec is much more simple without inline functions included. The exactly
same code sequence should run between different implantations. E.g. is the
behavior the function above the same if app is built with one set of compiler
flags and the two implementation with another sets of flags...
-Petri
