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.
> > 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? 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. > 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
