Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/odp_packet.c
line 72
@@ -334,6 +334,54 @@ void *_odp_packet_map(void *pkt_ptr, uint32_t offset,
uint32_t *seg_len,
return packet_map(pkt_ptr, offset, seg_len, seg_idx);
}
+int _odp_packet_copy_from_mem_seg(odp_packet_t pkt, uint32_t offset,
+ uint32_t len, const void *src)
+{
+ void *mapaddr;
+ uint32_t seglen = 0; /* GCC */
+ uint32_t cpylen;
+ const uint8_t *srcaddr = (const uint8_t *)src;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (offset + len > pkt_hdr->frame_len)
+ return -1;
+
+ while (len > 0) {
+ mapaddr = packet_map(pkt_hdr, offset, &seglen, NULL);
+ cpylen = len > seglen ? seglen : len;
+ memcpy(mapaddr, srcaddr, cpylen);
+ offset += cpylen;
+ srcaddr += cpylen;
+ len -= cpylen;
+ }
+
+ return 0;
+}
+
+int _odp_packet_copy_to_mem_seg(odp_packet_t pkt, uint32_t offset,
+ uint32_t len, void *dst)
+{
+ void *mapaddr;
+ uint32_t seglen = 0; /* GCC */
+ uint32_t cpylen;
+ uint8_t *dstaddr = (uint8_t *)dst;
+ odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);
+
+ if (offset + len > pkt_hdr->frame_len)
+ return -1;
+
+ while (len > 0) {
+ mapaddr = packet_map(pkt_hdr, offset, &seglen, NULL);
+ cpylen = len > seglen ? seglen : len;
+ memcpy(dstaddr, mapaddr, cpylen);
+ offset += cpylen;
+ dstaddr += cpylen;
+ len -= cpylen;
+ }
+
+ return 0;
+}
Comment:
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_r165113251
updated_at 2018-01-31 16:44:08
