On 01/24 08:13:27, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>
> > -----Original Message-----
> > From: Brian Brooks [mailto:brian.bro...@linaro.org]
> > Sent: Tuesday, January 24, 2017 9:25 AM
> > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> > labs.com>
> > Cc: lng-odp@lists.linaro.org
> > Subject: Re: [lng-odp] [PATCH v2 1/8] abi: event: added the first ABI spec
> > file
> >
> > On 01/20 16:11:56, Petri Savolainen wrote:
> > > Used event API as the first example of an ABI spec file. Used
> > > the same default architecture file initially for all
> > > architectures. Default ABI files avoid multiple copies
> > > of the same definition.
> >
> > Please consider an alternative approach to achieving API and ABI found
> > in the patch that I just posted to the ML. It does this by removing a
> > lot of code instead of adding a lot of code like this patchseries will.
>
> API and ABI are different specifications. API leaves room for different
> implementations. ABI fixes some of that variability (mainly type sizes) so
> that the same application image may run on different implementations of the
> same ABI spec (same CPU architecture).
If 100% of the C code for the types and function declarations existed in the
API header files there would be no unecessarily complex variability that you're
referring to. There would also be no need or confusion about API and ABI being
different specifications; they are not different specifications. The problem is
that not all of the C code is in the right header files, not that we haven't
finished our job in specifying things (or what we happen to be calling ABI).
> Implementations are free to support ABI or not. When an implementation does
> not support an ABI, everything left unspecified in API is implementation
> specific.
This is what happens when things are left unspecified in the API header files,
the implementations do this:
int do_work(void *obj);
int do_work(u64 obj);
bool do_work(void *obj);
The way to resolve this is to choose 1 and stick it in the API header file!
> When an implementation does support an ABI spec, it must agree on the
> definitions in the spec (type sizes).
If the type sizes were specified in the API and the expected C code exists in
the header files, this issue goes away. It's that simple.
> Different architectures may end up different compromises and different
> content of ABI spec files. E.g. x86-64 ABI could / should ensure that all
> types are aligned with DPDK, whereas arm64 ABI should reflect a compromise
> made by all armv8 SoC vendors. Thus e.g. type sizes between those two ABIs
> may be different.
>
> My series introduces first the ABI spec files.
> In this first phase, content of those files (type sizes) is copied from
> odp-linux so that number of changes is minimized. Later on when we have all
> ABI spec files in place. We'll clean out both the default ABI spec and
> odp-linux implementation. I assume that odp-linux will just use ABI spec
> definitions, in both ABI compat and non-compat modes. Other implementations
> should be more optimized and take everything out of the ABI non-compat mode.
>
> It makes sense to do ABI spec support in couple of steps, since ABI spec
> needs to define small things like value of various XXX_INVALID handles. I'd
> expect we'll change those to zero (from -1, 0xffffff, ~0, ...).
This is API. Choose 1 and make a portable decision.
> That small change alone, over the entire code base, may introduce or surface
> new bugs. So, it's better to introduce the mechanism first, and then
> gradually do specification work and required functional changes.
>
>
> First glimpse to your patch:
>
> diff --git a/include/odp/api/spec/atomic.h b/include/odp/api/spec/atomic.h
> index 408829df..d201d5bb 100644
> --- a/include/odp/api/spec/atomic.h
> +++ b/include/odp/api/spec/atomic.h
> @@ -74,7 +74,10 @@ extern "C" {
> * @param atom Pointer to atomic variable
> * @param val Value to initialize the variable with
> */
> -void odp_atomic_init_u32(odp_atomic_u32_t *atom, uint32_t val);
> +static inline void odp_atomic_init_u32(odp_atomic_u32_t *atom, uint32_t val)
> +{
> + __atomic_store_n(&atom->v, val, __ATOMIC_RELAXED);
> +}
>
>
> Now, this is an API spec file fixing the implementation,
This is OK. Static inlines are OK. So is 'fixing the implementation' for
trivial APIs.
> and actually requiring (by the spec) GCC built in support,
This is OK.
> which may change any time... Cannot do this.
Yes this is OK, versioning solves this problem.
> API spec is a standard, an interface specification - not code.
Yes, it is C code. And 100% of the types and function declarations need to be
there.
> ABI spec could include some pieces of code, but I'd be very careful on that
> so that correct functionality can be guaranteed always. For example, in this
> case direct assembly could be needed instead of compiler built-in, or can you
> guarantee that all suitable GCC versions, with various flag combinations
> produce always code that have identical functionality? E.g. some compiler
> flag may turn all __atomic_xxx to use a lock, instead of direct atomic
> instruction... which would break the interface between two binaries.
>
> -Petri
