[SYCL] Avoid strict aliasing violation when reading from byte arrays #5537
Conversation
When reading properties produced by sycl-post-link, the runtime will cast a byte-array pointer to another pointer of a diffrent type and read from the resulting pointer. This is a violation of the strict aliasing rule and as such is UB. These changes replace the violating reads with memcpys from the byte arrays. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
| auto *It = reinterpret_cast<const std::uint32_t *>(&Descriptors[8]); | ||
| auto *End = reinterpret_cast<const std::uint32_t *>(&Descriptors[0] + | ||
| Descriptors.size()); | ||
| const uint8_t *It = &Descriptors[8]; |
There was a problem hiding this comment.
DeviceBinaryProperty has a (A: no. that's for a single uint32, not an array like the other ).asUint32() method. Couldn't we call that above, instead of .asByteArray() and then avoid the reinterpret cast on that front?
Alternately, instead of inserting std::memcpy down below, I wonder if we could use sycl::bit_cast ?
I know the reinterpret cast for pointers means we potentially have UB, but given that we are getting raw binary data from the PI, are we really avoiding any issues by switching from reinterpet_cast to memcpy into local vars?
There was a problem hiding this comment.
Alternately, instead of inserting
std::memcpydown below, I wonder if we could usesycl::bit_cast?
I believe sycl::bit_cast asserts that To and From have the same size, so we can't use it here sadly.
I know the reinterpret cast for pointers means we potentially have UB, but given that we are getting raw binary data from the PI, are we really avoiding any issues by switching from reinterpet_cast to memcpy into local vars?
Currently there isn't must difference between the two, except we assume that the data sycl-post-link generates has the same structure as the types we use. If this starts failing it is on us. However, relying on UB means we are at the compiler's mercy and any day it could change. Do I think it's likely to happen? No, but it would likely be a huge headache if it ever does and making this change seems like a small price to avoid future pains. Better safe than sorry!
bader
requested review from
cperkinsintel
and removed request for
alexbatashev
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
|
Per offline discussion with @aelovikov-intel this is being moved to draft to make further improvements. |
An alternative approach: #7023. |
|
#7023 has been merged. This is no longer needed. |
When reading properties produced by sycl-post-link, the runtime will cast a byte-array pointer to another pointer of a diffrent type and read from the resulting pointer. This is a violation of the strict aliasing rule and as such is UB. These changes replace the violating reads with memcpys from the byte arrays.
Note: This is a code-quality change and should have no effect on current stability. Thank you @psamolysov-intel for pointing out the UB.