OK. As I said, we're not the first project to run into this and GCC is pretty clever in its optimizations for the most part. So except for the question of handle equality tests, these changes are transparent both functionally and performance-wise to current ODP code and applications, which is why I'm proposing them.
On Mon, Dec 15, 2014 at 12:14 PM, Ola Liljedahl <[email protected]> wrote: > > On 15 December 2014 at 18:46, Bill Fischofer <[email protected]> > wrote: > > > > > > On Mon, Dec 15, 2014 at 11:32 AM, Ola Liljedahl < > [email protected]> > > wrote: > >> > >> On 15 December 2014 at 18:10, Bill Fischofer <[email protected] > > > >> wrote: > >> > > >> > 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. > >> Yes. > >> > >> I checked for ARM and GCC seems to do a good job with structs that > >> contain a 32-bit integer when passing those structs by value or > >> returning such structs (e.g. from alloc-type functions), no pointers > >> involved. So from a performance perspective I have nothing to complain > >> about. Maybe this isn't such a bad idea as I first thought... > > > > > > That's been my experience as well. GCC understands how to optimize these > > sort of small structs. I suspect it would do equally well for a struct > that > > wrappered a uint64_t value; > > > >> > >> > >> typedef struct { int x; } handle_t; > > > > > > No! You cannot do this. I tried this at first and again this falls > prey to > This was *not* an example of some generic ODP handle. This was just > the code snippet I used to check how small structs are handled by GCC. > I though it might be useful for other people who might want to test > this with their favourite compiler for their target. > > > C's weak typedef system. Making everything a typedef'd handle_t is no > > different than making them a typedef'd uint32_t. C would still regard > all > > handle types as interchangeable. To get strong typing you have to say: > > > > #define handle_t struct { int x; } > > > > I used #define for the xxx_INVALIDs to be consistent with what we're > > currently using, but static const should be equally valid. > If this constant handle is not a const variable, you will probably > force the compiler to load the value of the variable every time the > constant is used. > > > > >> > >> static const handle_t INVALID = { -1 }; > >> > >> handle_t invalid(void) > >> { > >> return INVALID; > >> } > >> > >> int compare(handle_t h1, handle_t h2) > >> { > >> return h1.x == h2.x; > >> } > > > > > > These need to be either macros or inline functions for performance. I > used > For these types of calls yes. But I was primarily interested in how > GCC would generate code for those calls where we won't provide macros > or inline functions. Which seem to be the majority of the functions in > the different ODP API's, at least for linux-generic. > > > #define for odp_equal(). How to include inline APIs is something we > need to > > formalize sometime in 2015 since it will be needed for any > production-grade > > ODP implementation. > > > >> > >> > >> handle_t alloc(unsigned idx) > >> { > >> handle_t h; > >> h.x = idx; > >> return h; > >> } > >> > >> > >> > > >> >> > >> >> > >> >> > > >> >> > 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. > >> A small mistake. Of course I meant "if (buf != ODP_BUFFER_INVALID) > >> odp_buffer_free(buf);". > >> > >> > > >> >> > >> >> > >> >> > 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
