Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 28
@@ -49,15 +49,14 @@ const _odp_packet_inline_offset_t ODP_ALIGNED_CACHE
_odp_packet_inline = {
#include <odp/visibility_end.h>
-static inline odp_buffer_t buffer_handle(odp_packet_hdr_t *pkt_hdr)
-{
- return (odp_buffer_t)pkt_hdr;
-}
-
-static inline odp_packet_hdr_t *buf_to_packet_hdr(odp_buffer_t buf)
-{
- return (odp_packet_hdr_t *)buf_hdl_to_hdr(buf);
-}
+/* Check that invalid values are the same. Some versions of Clang have trouble
+ * with the strong type casting, and complain that these invalid values are not
+ * integral constants. */
+#ifndef __clang__
+ODP_STATIC_ASSERT(ODP_PACKET_INVALID == 0, "Packet invalid not 0");
+ODP_STATIC_ASSERT(ODP_BUFFER_INVALID == 0, "Buffer invalid not 0");
+ODP_STATIC_ASSERT(ODP_EVENT_INVALID == 0, "Event invalid not 0");
Comment:
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_r165108040
updated_at 2018-01-31 16:28:26
