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
