Petri Savolainen(psavol) replied on github web page:
test/validation/api/pktio/pktio.c
line 16
@@ -1195,10 +1195,8 @@ void pktio_test_index(void)
ndx = odp_pktio_index(pktio);
CU_ASSERT(ndx >= 0);
- CU_ASSERT(odp_pktio_index(pktio_inval) < 0);
CU_ASSERT(odp_pktio_close(pktio) == 0);
- CU_ASSERT(odp_pktio_index(pktio) < 0);
Comment:
API does not (and should not) guarantee that invalid or stale handles are
checked on fast path functions, such as this conversion to index. Especially,
any API function is not specified to check against stale handles. Function may
return -1, if it finds an error but we don't guarantee which errors are
checked. For example, there may be check against invalid handle, but there may
not be either. Invalid values are mostly needed for API function to be able
return that e.g. create/open/alloc failed. E.g. here, application should check
that pktio open succeeded, if it did not succeed it should not continue and
pass that invalid handle to other pktio calls. That way every API call does not
need to check the handle, only application needs to check it once after
open/create/alloc.
This close + index test above it basically the same thing as doing:
odp_packet_free(pkt);
data = odp_packet_data(pkt);
// now we can assume that data is NULL, since pkt packet was freed and this
function always searches through valid handles and notices that this handle
value is not valid anymore.
> Petri Savolainen(psavol) wrote:
> Those need to be inside #include <odp/visibility_begin.h> #include
> <odp/visibility_end.h>. It's cleaner to have one vs multiple such regions in
> one c file.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> We can have `_odp_pktio_index()` which does not perform such checks and
>> `odp_pktio_index()` which does `ODP_PKTIO_INVALID` check.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Generally I would agree. However we explicitly have the invalid usecase in
>>> API:
>>> ```
>>> @retval <0 On failure (e.g., handle not valid)
>>> ```
>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>> Is there a reason to move function bodies?
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> Maybe `(uintptr_t)(handle) == (uintptr_t)0` would do the trick?
>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>> Very strange. I'm Ok with this workaround, but a comment about clang
>>>>>> version might be appropriate. Don't know if it's worth doing a clang
>>>>>> version check. We do things like that in the `ODP_STATIC_ASSERT()` macro
>>>>>> itself for similar reasons.
>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>> odp_packet.c:52:19: error: static_assert expression is not an integral
>>>>>>> constant
>>>>>>> expression
>>>>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_BUFFER_INVALID) == 0, "Buffer inval
>>>>>>> not 0");
>>>>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:30:
>>>>>>> note:
>>>>>>> expanded from macro '_odp_typeval'
>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>> ^
>>>>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded from
>>>>>>> macro
>>>>>>> 'ODP_STATIC_ASSERT'
>>>>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>>>>> ^~~~
>>>>>>> odp_packet.c:52:19: note: cast that performs the conversions of a
>>>>>>> reinterpret_cast is not allowed in a constant expression
>>>>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:41:
>>>>>>> note:
>>>>>>> expanded from macro '_odp_typeval'
>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>> static_assert() seems special. The clang version in Travis does not
>>>>>>>> accept type casts with static_assert(). The cast confuses it to think
>>>>>>>> that (uintptr_t)0 == 0 is not an integral constant.
>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>> `_odp_typeval()` is defined as:
>>>>>>>>> ```
>>>>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>>>>> ```
>>>>>>>>> It's used elsewhere in the ODP code and clang doesn't have any
>>>>>>>>> problem with it. I've verified this works for clang 4.2.1. Since it
>>>>>>>>> works elsewhere on Travis I'm not sure why it wouldn't in this
>>>>>>>>> instance.
>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>> I'm OK with just a modified commit name. I agree we needn't be too
>>>>>>>>>> granular about these things.
>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>> I could rename the patch to:
>>>>>>>>>>> "linux-gen: packet: use inlined flow hash and ts set"
>>>>>>>>>>>
>>>>>>>>>>> ... if necessary. The comment says already that also ts set is
>>>>>>>>>>> modified the same way.
>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>> I did try several combinations of type casts of invalid values,
>>>>>>>>>>>> also cast to uintptr_t. The problem is that gcc and clang (clang
>>>>>>>>>>>> version 3.8.0-2ubuntu4) of my machine accepted all of those casts,
>>>>>>>>>>>> but the clang version in Travis (clang version
>>>>>>>>>>>> 3.8.0-2ubuntu3~trusty5) does not accept anything.
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe it's a bug in the particular version of clang. Anyway,
>>>>>>>>>>>> addition of this check even only on gcc side is improvement
>>>>>>>>>>>> against what we currently have (no check). Packet invalid is
>>>>>>>>>>>> currently 0xfffffff in ABI spec, but event/buffer invalid are 0.
>>>>>>>>>>>>
>>>>>>>>>>>> odp_packet.c:55:19: error: static_assert expression is not an
>>>>>>>>>>>> integral constant
>>>>>>>>>>>> expression
>>>>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded
>>>>>>>>>>>> from macro
>>>>>>>>>>>> 'ODP_STATIC_ASSERT'
>>>>>>>>>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>>>>>>>>>> ^~~~
>>>>>>>>>>>> odp_packet.c:55:20: note: cast that performs the conversions of a
>>>>>>>>>>>> reinterpret_cast is not allowed in a constant expression
>>>>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>> ^
>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>> Since you want to breech the strong type you need to use the
>>>>>>>>>>>>> internal `odp_typeval()` macro here.
>>>>>>>>>>>>> ```
>>>>>>>>>>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_EVENT_INVALID) == 0, "Event
>>>>>>>>>>>>> invalid not 0");
>>>>>>>>>>>>> ```
>>>>>>>>>>>>> etc. I've verified that this works fine for clang, so no need for
>>>>>>>>>>>>> the conditional compilation.
>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>> Should this be in its own commit? Different function.
https://github.com/Linaro/odp/pull/437#discussion_r165279708
updated_at 2018-02-01 07:53:01
