bkietz commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806672308
##########
cpp/src/arrow/util/ubsan.h:
##########
@@ -63,7 +63,7 @@ inline std::enable_if_t<std::is_trivially_copyable_v<T>, T>
SafeLoadAs(
template <typename T>
inline std::enable_if_t<std::is_trivially_copyable_v<T>, T> SafeLoad(const T*
unaligned) {
std::remove_const_t<T> ret;
- std::memcpy(&ret, unaligned, sizeof(T));
+ std::memcpy(&ret, reinterpret_cast<const void*>(unaligned), sizeof(T));
Review Comment:
In C it's undefined behavior to **materialize** an unaligned pointer whereas
in C++ it's only undefined to access such. The new assertion in ubsan is a nice
example of compiler optimizations encroaching into previously "safe" undefined
behavior; since memcpy's parameter is `void const*` it initially appears that
memcpy shouldn't care about the pointer's alignment. However, since the
compiler knows that we're accessing the pointer and can therefore assume that
it points to aligned memory... the memcpy call can always be replaced by an
aligned load.
In light of this, I think we probably want to throw a `std::launder` in here
as well- otherwise in the future, the compiler could trace through the cast and
make the same breaking assumption again
```suggestion
std::memcpy(&ret, std::launder(static_cast<const void*>(unaligned)),
sizeof(T));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
