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