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:
To me "(e.g. handle not valid)" does not yet guarantee the check. This would:
"<0 When pktio handle is invalid, or some other error happened".
In general, we should remove all such "e.g. handle not valid" comments.
> Petri Savolainen(psavol) wrote:
> 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_r165280121
updated_at 2018-02-01 07:55:59
