On Mon, Dec 15, 2014 at 11:32 AM, Ola Liljedahl <[email protected]>
wrote:
>
> On 15 December 2014 at 18:10, Bill Fischofer <[email protected]>
> wrote:
> >
> > On Mon, Dec 15, 2014 at 10:58 AM, Ola Liljedahl <
> [email protected]>
> > wrote:
> >>
> >> On 15 December 2014 at 17:21, Bill Fischofer <[email protected]
> >
> >> wrote:
> >> > The code is just a translation of existing code. If the existing code
> >> > said
> >> > == I replaced that with odp_equal().
> >> Which doesn't work when one parameter is ODP_XXX_INVALID unless this
> >> is defined a struct with some invalid value stored inside.
> >
> >
> > Yes, in the repo I mentioned ODP_xxx_INVALID are correctly typed as
> handles
> > and work as written.
> > These are output values from ODP APIs that return handle types and so
> must
> > be appropriately typed.
> Yes.
>
> I checked for ARM and GCC seems to do a good job with structs that
> contain a 32-bit integer when passing those structs by value or
> returning such structs (e.g. from alloc-type functions), no pointers
> involved. So from a performance perspective I have nothing to complain
> about. Maybe this isn't such a bad idea as I first thought...
>
That's been my experience as well. GCC understands how to optimize these
sort of small structs. I suspect it would do equally well for a struct
that wrappered a uint64_t value;
>
> typedef struct { int x; } handle_t;
>
No! You cannot do this. I tried this at first and again this falls prey
to C's weak typedef system. Making everything a typedef'd handle_t is no
different than making them a typedef'd uint32_t. C would still regard all
handle types as interchangeable. To get strong typing you have to say:
#define handle_t struct { int x; }
I used #define for the xxx_INVALIDs to be consistent with what we're
currently using, but static const should be equally valid.
> static const handle_t INVALID = { -1 };
>
> handle_t invalid(void)
> {
> return INVALID;
> }
>
> int compare(handle_t h1, handle_t h2)
> {
> return h1.x == h2.x;
> }
>
These need to be either macros or inline functions for performance. I used
#define for odp_equal(). How to include inline APIs is something we need
to formalize sometime in 2015 since it will be needed for any
production-grade ODP implementation.
>
> handle_t alloc(unsigned idx)
> {
> handle_t h;
> h.x = idx;
> return h;
> }
>
>
> >
> >>
> >>
> >> >
> >> > There is a difference between testing the value of what's returned
> form
> >> > an
> >> > ODP API vs. validating a potentially suspect handle. For the former,
> >> > odp_equal() is a lot more efficient than doing xxx_is_valid() calls
> >> > since
> >> > the latter is designed to do robust validation and is not intended for
> >> > use
> >> Oh I wasn't thinking that we should revive those calls (which I had
> >> completely forgotten about). I understood Maxim's suggestion as all
> >> current usage of "handle1 == handle2" was actually of the form "handle
> >> == ODP_XXX_INVALID" and thus we only need a function for checking
> >> against ODP_XXX_INVALID, not a generic comparison function. Did I
> >> misunderstand Maxim here?
> >
> >
> > No, in many cases the code is comparing handles to each other, not just
> to
> > the xxx_INVALID
> > special handles. So a general equality test is needed. The
> xxx_is_valid()
> > calls are overkill for
> > this purpose as previously noted and are designed to serve a different
> > purpose.
> >
> >>
> >>
> >> In many situations you want to be able to compare the value of a
> >> variable with the INVALID (NULL) value. E.g.
> >> if (buf == ODP_BUFFER_INVALID) odp_buffer_free(buf);
> >> which could be written as
> >> if (odp_buffer_invalid(buf)) odp_buffer_free(buf);
> >>
> >> Comparing two arbitrary values is possibly less likely.
> >
> >
> > Not sure what the intent of that example is. As written neither form
> makes
> > sense since you can only
> > free a valid buffer.
> A small mistake. Of course I meant "if (buf != ODP_BUFFER_INVALID)
> odp_buffer_free(buf);".
>
> >
> >>
> >>
> >> > in performance paths. That's why we've said that ODP APIs in general
> do
> >> > not
> >> > do extensive validation of their input parameters.
> >> >
> >> > On Mon, Dec 15, 2014 at 10:06 AM, Ola Liljedahl
> >> > <[email protected]>
> >> > wrote:
> >> >>
> >> >> On 15 December 2014 at 16:07, Maxim Uvarov <[email protected]>
> >> >> wrote:
> >> >> > On 12/15/2014 04:18 PM, Ola Liljedahl wrote:
> >> >> >>
> >> >> >> On 15 December 2014 at 14:02, Bill Fischofer
> >> >> >> <[email protected]>
> >> >> >> wrote:
> >> >> >>>
> >> >> >>> I have a proof-of-concept implementation of this idea available
> at
> >> >> >>> http://git.linaro.org/people/bill.fischofer/odp_strongtypes.git
> and
> >> >> >>> it
> >> >> >>> doesn't appear to suffer any performance degradation compared to
> >> >> >>> the
> >> >> >>> weak
> >> >> >>> type version.
> >> >> >
> >> >> >
> >> >> > In a lot of places:
> >> >> > if (odp_equal(pkt, ODP_PACKET_INVALID))
> >> >> >
> >> >> > I think we need odp_packet_valid() function. == or odp_equal is
> not
> >> >> > needed
> >> >> > for such cases.
> >> >> Good suggestion. Does this support 100% of cases (where == is used)
> >> >> today or just 99%?
> >> >>
> >> >> Let's start by adding such functions to the different API's.
> >> >> odp_xxx_equal() can come later.
> >> >>
> >> >> >
> >> >> > Looks like everywhere you test is it valid or not. So you don't
> need
> >> >> > ==
> >> >> > for
> >> >> > that.
> >> >> >
> >> >> > Maxim.
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> >>> If we can get sparse or some other tool to do this I'm fine with
> >> >> >>> that
> >> >> >>> approach, but this issue needs to be addressed by some means. I
> >> >> >>> think
> >> >> >>> it's
> >> >> >>> simpler if the C compiler can do it by itself rather than
> requiring
> >> >> >>> some
> >> >> >>> other tool that may not be used or used with any regularity.
> >> >> >>>
> >> >> >>> The other issue is just how abstract are the ODP abstract types?
> >> >> >>> We've
> >> >> >>> hand-waved this issue but as currently written the code will fail
> >> >> >>> if
> >> >> >>> someone
> >> >> >>> tries to define handles as anything other than 32-bit quantities.
> >> >> >>
> >> >> >> I expect some more performance and hardware oriented ODP
> >> >> >> implementations would use pointers for e.g. the buffer derived
> >> >> >> types.
> >> >> >>
> >> >> >> To what extent does this strict type checking affect other ODP
> >> >> >> implementations from using scalar or pointer types for handles? An
> >> >> >> implementation can always provide its own type definitions but
> must
> >> >> >> support the additional functions we defined (e.g. compare or
> >> >> >> is_equal
> >> >> >> functions).
> >> >> >>
> >> >> >>> On Mon, Dec 15, 2014 at 6:03 AM, Ola Liljedahl
> >> >> >>> <[email protected]>
> >> >> >>> wrote:
> >> >> >>>>
> >> >> >>>> On 15 December 2014 at 12:51, Mike Holmes <
> [email protected]>
> >> >> >>>> wrote:
> >> >> >>>>>
> >> >> >>>>>
> >> >> >>>>> On 15 December 2014 at 06:26, Ola Liljedahl
> >> >> >>>>> <[email protected]>
> >> >> >>>>> wrote:
> >> >> >>>>>>
> >> >> >>>>>> On 13 December 2014 at 21:20, Bill Fischofer
> >> >> >>>>>> <[email protected]>
> >> >> >>>>>> wrote:
> >> >> >>>>>>>
> >> >> >>>>>>> Background:
> >> >> >>>>>>>
> >> >> >>>>>>> In the ODP linux-generic implementation we're currently
> >> >> >>>>>>> defining
> >> >> >>>>>>> ODP
> >> >> >>>>>>> abstract types like this:
> >> >> >>>>>>>
> >> >> >>>>>>> typedef uint32_t odp_buffer_t;
> >> >> >>>>>>> typedef uint32_t odp_buffer_pool_t;
> >> >> >>>>>>> typedef uint32_t odp_packet_t;
> >> >> >>>>>>> ...etc.
> >> >> >>>>>>>
> >> >> >>>>>>> While this provides documentation, unfortunately typedefs in
> C
> >> >> >>>>>>> are
> >> >> >>>>>>> weak,
> >> >> >>>>>>> meaning that although odp_buffer_t and odp_buffer_pool_t may
> >> >> >>>>>>> look
> >> >> >>>>>>> different,
> >> >> >>>>>>> C will treat them as equivalent since they are just aliases
> for
> >> >> >>>>>>> uint32_t.
> >> >> >>>>>>> This was brought home in a recent bug we detected in
> >> >> >>>>>>> odp_crypto.c.
> >> >> >>>>>>>
> >> >> >>>>>>> As part of the API normalization work for ODP v1.0 the syntax
> >> >> >>>>>>> and
> >> >> >>>>>>> semantics
> >> >> >>>>>>> of odp_packet_copy() were changed from:
> >> >> >>>>>>>
> >> >> >>>>>>> int odp_packet_copy(odp_packet_t pkt_dst, odp_packet_t
> >> >> >>>>>>> pkt_src);
> >> >> >>>>>>>
> >> >> >>>>>>> to
> >> >> >>>>>>>
> >> >> >>>>>>> odp_packet_t odp_packet_copy(odp_packet_t pkt,
> >> >> >>>>>>> odp_buffer_pool_t
> >> >> >>>>>>> pool);
> >> >> >>>>>>>
> >> >> >>>>>>> However, odp_crypto.c contained the line:
> >> >> >>>>>>>
> >> >> >>>>>>> odp_packet_copy(params->out_pkt, params->pkt);
> >> >> >>>>>>>
> >> >> >>>>>>> which C happily accepted without comment even though it's
> >> >> >>>>>>> obviously
> >> >> >>>>>>> very
> >> >> >>>>>>> wrong under the new definition of the API. With strong
> typing,
> >> >> >>>>>>> this
> >> >> >>>>>>> would
> >> >> >>>>>>> be flagged at compile time, which is clearly preferable to
> >> >> >>>>>>> letting
> >> >> >>>>>>> things
> >> >> >>>>>>> like this slip by.
> >> >> >>>>>>>
> >> >> >>>>>>> Strong typing in C:
> >> >> >>>>>>>
> >> >> >>>>>>> There are a number of techniques to achieve strong typing in
> C,
> >> >> >>>>>>> but
> >> >> >>>>>>> the
> >> >> >>>>>>> most
> >> >> >>>>>>> straightforward way to make typedefs strong is by changing
> the
> >> >> >>>>>>> type
> >> >> >>>>>>> definitions to the following:
> >> >> >>>>>>>
> >> >> >>>>>>> #define odp_handle_t struct { uint32_t val; }
> >> >> >>>>>>
> >> >> >>>>>> This might have performance implications. Often structs are
> >> >> >>>>>> passed
> >> >> >>>>>> by
> >> >> >>>>>> reference (a pointer to a temporary struct) and not by value
> >> >> >>>>>> even
> >> >> >>>>>> when
> >> >> >>>>>> the struct is so small it could easily have fitted into
> >> >> >>>>>> registers.
> >> >> >>>>>> Same for returning structs. The calling convention (ABI) of
> the
> >> >> >>>>>> target
> >> >> >>>>>> decides.
> >> >> >>>>>>
> >> >> >>>>>>
> >> >> >>>>>>> typedef odp_handle_t odp_buffer_t;
> >> >> >>>>>>> typedef odp_handle_t odp_buffer_pool_t;
> >> >> >>>>>>> typedef odp_handle_t odp_packet_t;
> >> >> >>>>>>> ...etc.
> >> >> >>>>>>>
> >> >> >>>>>>> Under these type definitions you cannot mix and match ODP
> types
> >> >> >>>>>>> without
> >> >> >>>>>>> some
> >> >> >>>>>>> sort of explicit type conversion, which is why the API
> provides
> >> >> >>>>>>> routines
> >> >> >>>>>>> like odp_packet_to_buffer() and odp_packet_from_buffer().
> >> >> >>>>>>> Moreover,
> >> >> >>>>>>> this
> >> >> >>>>>>> change is transparent for the most part to existing correct
> ODP
> >> >> >>>>>>> applications, so that's all good.
> >> >> >>>>>>>
> >> >> >>>>>>> The only downside is that with weak types today you can write
> >> >> >>>>>>> code
> >> >> >>>>>>> like:
> >> >> >>>>>>>
> >> >> >>>>>>> if (buf == ODP_BUFFER_INVALID) ...
> >> >> >>>>>>>
> >> >> >>>>>>> to check things like the return value from
> odp_buffer_alloc().
> >> >> >>>>>>> However
> >> >> >>>>>>> with
> >> >> >>>>>>> strong types C will reject that since operators like ==
> require
> >> >> >>>>>>> numeric
> >> >> >>>>>>> operands and unlike C++, C does not support operator
> >> >> >>>>>>> overloading.
> >> >> >>>>>>> This
> >> >> >>>>>>> is
> >> >> >>>>>>> easily remedied by including a simple macro:
> >> >> >>>>>>>
> >> >> >>>>>>> #define odp_equal(x, y) (x.val == y.val)
> >> >> >>>>>>>
> >> >> >>>>>>> in odp_platform_types.h so that the application can write:
> >> >> >>>>>>>
> >> >> >>>>>>> if (odp_equal(buf, ODP_BUFFER_INVALID)) ...
> >> >> >>>>>>
> >> >> >>>>>> You need another macro for comparing the value of some ODP
> >> >> >>>>>> handle
> >> >> >>>>>> with
> >> >> >>>>>> a constant value like ODP_BUFFER_INVALID (unless
> >> >> >>>>>> ODP_BUFFER_INVALID
> >> >> >>>>>> itself is a handle of the struct kind).
> >> >> >>>>>>
> >> >> >>>>>>> instead.
> >> >> >>>>>>>
> >> >> >>>>>>> However this does require a source code change on the part of
> >> >> >>>>>>> the
> >> >> >>>>>>> application, hence this RFC. I believe this is a very small
> >> >> >>>>>>> price
> >> >> >>>>>>> to
> >> >> >>>>>>> pay
> >> >> >>>>>>> for
> >> >> >>>>>>> the improved type safety and would like to propose that we
> >> >> >>>>>>> switch
> >> >> >>>>>>> to
> >> >> >>>>>>> strong
> >> >> >>>>>>> typing prior to the official ODP v1.0 release, which would
> mean
> >> >> >>>>>>> officially
> >> >> >>>>>>> adding and documenting odp_equal() as an ODP API.
> >> >> >>>>>>>
> >> >> >>>>>>> Comments, please.
> >> >> >>>>>>
> >> >> >>>>>> I think this is a problem best solved by other tool like
> sparse,
> >> >> >>>>>> not
> >> >> >>>>>> by the C compiler. Or if we think the compiler itself should
> do
> >> >> >>>>>> these
> >> >> >>>>>> types of checks (need a good argument for that), then we
> should
> >> >> >>>>>> migrate to C++.
> >> >> >>>>>
> >> >> >>>>>
> >> >> >>>>> The gimple lint tool is capable of distinguishing the weak
> types,
> >> >> >>>>> did
> >> >> >>>>> anyone
> >> >> >>>>> try passing this issue through coverity to see if it is capable
> >> >> >>>>> of
> >> >> >>>>> making
> >> >> >>>>> the distinction ?
> >> >> >>>>> I'd rather find a tool than create unusual C code.
> >> >> >>>>
> >> >> >>>> Perhaps splint. It has some kind of support for "abstract"
> types.
> >> >> >>>> The
> >> >> >>>> basic idea seems to be to prevent users of abstract types to
> >> >> >>>> access
> >> >> >>>> implementation-specific details. I don't know if it detects the
> >> >> >>>> mixing
> >> >> >>>> if different abstract types but strong type checking is one
> >> >> >>>> feature
> >> >> >>>> of
> >> >> >>>> splint.
> >> >> >>>>
> >> >> >>>>>>
> >> >> >>>>>>>
> >> >> >>>>>>>
> >> >> >>>>>>> _______________________________________________
> >> >> >>>>>>> lng-odp mailing list
> >> >> >>>>>>> [email protected]
> >> >> >>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >> >> >>>>>>>
> >> >> >>>>>> _______________________________________________
> >> >> >>>>>> lng-odp mailing list
> >> >> >>>>>> [email protected]
> >> >> >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >> >> >>>>>
> >> >> >>>>>
> >> >> >>>>>
> >> >> >>>>> --
> >> >> >>>>> Mike Holmes
> >> >> >>>>> Linaro Sr Technical Manager
> >> >> >>>>> LNG - ODP
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> lng-odp mailing list
> >> >> >> [email protected]
> >> >> >> http://lists.linaro.org/mailman/listinfo/lng-odp
> >> >> >
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > lng-odp mailing list
> >> >> > [email protected]
> >> >> > http://lists.linaro.org/mailman/listinfo/lng-odp
> >> >>
> >> >> _______________________________________________
> >> >> lng-odp mailing list
> >> >> [email protected]
> >> >> http://lists.linaro.org/mailman/listinfo/lng-odp
>
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
