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...
typedef struct { int x; } handle_t;
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;
}
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
