We've said an implementation is free to define ODP abstract types however it wishes. If I were an implementer, then defining these handles strongly as shown here would be legitimate by what we've said, however it would not work since the current API assumes that == can be used on handles directly. This suggests that we *should* be using odp_equal() rather than == independent of how we choose to implement these typedefs ourselves in linux-generic.
On Mon, Dec 15, 2014 at 7:18 AM, Ola Liljedahl <[email protected]> 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. > > > > 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
