OK. For the v2 series: Reviewed-and-tested-by: Bill Fischofer <[email protected]>
On Mon, Jan 23, 2017 at 5:09 AM, Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> wrote: >> > +#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 > > >
