You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2024/10/18 09:46:34 UTC
[PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
pitrou opened a new pull request, #44468:
URL: https://github.com/apache/arrow/pull/44468
### Rationale for this change
CLang 18 complains about undefined behavior when memcpy is called on a T-unaligned `T*` pointer.
This is due to this upstream change: https://github.com/llvm/llvm-project/pull/67766
### What changes are included in this PR?
Workaround by casting to `void*`.
### Are these changes tested?
By existing CI tests.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806749389
##########
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:
I think I'd rather merge without `std::launder` for now. We can reevaluate later if the issue arises, IMHO.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44468:
URL: https://github.com/apache/arrow/pull/44468#issuecomment-2422702010
Revision: ab378dd937e56388e029f6c988c46aea74b48ff7
Submitted crossbow builds: [ursacomputing/crossbow @ actions-be2686045c](https://github.com/ursacomputing/crossbow/branches/all?query=actions-be2686045c)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290721/job/31739716619)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290757/job/31739717052)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/11406289997/job/31739713573)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406289992/job/31739713821)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290352/job/31739715481)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290412/job/31739715491)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290338/job/31739715442)|
|test-cuda-cpp-ubuntu-20.04-cuda-11.2.2|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290063/job/31739713956)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290528/job/31739716124)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290217/job/31739714927)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290144/job/31739714558)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290226/job/31739715005)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290643/job/31739716472)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290114/job/31739714576)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406289861/job/31739713477)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290522/job/31739716016)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/11406289918/job/31739713695)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290581/job/31739716182)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/11406291173/job/31739718323)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/11406291062/job/31739718331)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/11406291244/job/31739718375)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290716/job/31739716592)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/11406290299/job/31739716183)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806451089
##########
cpp/src/arrow/util/ubsan.h:
##########
@@ -73,7 +73,7 @@ inline std::enable_if_t<std::is_trivially_copyable_v<T> &&
U>
SafeCopy(T value) {
std::remove_const_t<U> ret;
- std::memcpy(&ret, &value, sizeof(T));
+ std::memcpy(&ret, reinterpret_cast<const void*>(&value), sizeof(T));
Review Comment:
This one surprises me. If value is on the stack, then it is already correctly aligned for T
##########
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:
This should probably just be static cast
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806661242
##########
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:
```suggestion
std::memcpy(&ret, 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806661472
##########
cpp/src/arrow/util/ubsan.h:
##########
@@ -73,7 +73,7 @@ inline std::enable_if_t<std::is_trivially_copyable_v<T> &&
U>
SafeCopy(T value) {
std::remove_const_t<U> ret;
- std::memcpy(&ret, &value, sizeof(T));
+ std::memcpy(&ret, reinterpret_cast<const void*>(&value), sizeof(T));
Review Comment:
```suggestion
std::memcpy(&ret, static_cast<const void*>(&value), 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #44468:
URL: https://github.com/apache/arrow/pull/44468
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44468:
URL: https://github.com/apache/arrow/pull/44468#issuecomment-2422697058
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44468:
URL: https://github.com/apache/arrow/pull/44468#issuecomment-2422003652
:warning: GitHub issue #44372 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806686950
##########
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:
Hmm, the doc for `std::launder` says:
> Devirtualization fence with respect to p. Returns a pointer to the same memory that p points to, but where the referent object is assumed to have a distinct lifetime and dynamic type.
Does the "distinct lifetime" imply that ASAN wouldn't be able to detect use-after-free?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806729464
##########
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:
Also, this does not seem to be one of the "typical uses of std::launder" as mentioned in https://en.cppreference.com/w/cpp/utility/launder
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806657258
##########
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:
static cast is more strict and therefore preferable here; reinterpret cast should only be used when the type system is being circumvented somehow (eg [type punning](https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/aligned_storage.h#L36))
The [legal operations for reinterpret cast](https://timsong-cpp.github.io/cppwp/n4868/expr.reinterpret.cast) don't include many of the things we use it for, but ... the type punning we do in arrow-c++ is not unique to this project.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806613497
##########
cpp/src/arrow/util/ubsan.h:
##########
@@ -73,7 +73,7 @@ inline std::enable_if_t<std::is_trivially_copyable_v<T> &&
U>
SafeCopy(T value) {
std::remove_const_t<U> ret;
- std::memcpy(&ret, &value, sizeof(T));
+ std::memcpy(&ret, reinterpret_cast<const void*>(&value), sizeof(T));
Review Comment:
Hmm, that's possible. I couldn't reproduce the original error...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44468:
URL: https://github.com/apache/arrow/pull/44468#discussion_r1806613935
##########
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:
What is the difference exactly in this case? Is `reinterpret_cast` stricter, or the converse?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44372: [C++] Fix unaligned load/store implementation for clang-18 [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #44468:
URL: https://github.com/apache/arrow/pull/44468#issuecomment-2423400352
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit a5850e1097515b9afd401a9bc6b265ced288c360.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/31759471159) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org