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