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
