Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Multiple definitions of std::hash<FieldPath> in unity builds #36214

Closed
benibus opened this issue Jun 21, 2023 · 0 comments · Fixed by #36222
Closed

[C++] Multiple definitions of std::hash<FieldPath> in unity builds #36214

benibus opened this issue Jun 21, 2023 · 0 comments · Fixed by #36222
Assignees
Milestone

Comments

@benibus
Copy link
Collaborator

benibus commented Jun 21, 2023

Describe the bug, including details regarding any error messages, version, and platform.

Discovered in an in-progress PR (#36073), but this will likely affect others soon if it hasn't already. In this case, it involves arrow/ipc/dictionary.cc and arrow/compute/kernels/vector_sort.cc.

Example error (from java-jni-manylinux-2014):

FAILED: src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx.o 
/usr/local/bin/ccache /opt/rh/devtoolset-10/root/usr/bin/c++ -DARROW_EXPORTING -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_S3_HAS_CRT -DARROW_WITH_BACKTRACE -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DAWS_MQTT_WITH_WEBSOCKETS -DAWS_SDK_VERSION_MAJOR=1 -DAWS_SDK_VERSION_MINOR=11 -DAWS_SDK_VERSION_PATCH=4 -DAWS_USE_EPOLL -DCURL_STATICLIB -DURI_STATIC_BUILD -DUTF8PROC_STATIC -I/build/cpp/src -I/arrow/cpp/src -I/arrow/cpp/src/generated -isystem /arrow/cpp/thirdparty/flatbuffers/include -isystem /arrow/cpp/thirdparty/hadoop/include -isystem /opt/vcpkg/installed/amd64-linux-static-release/include -isystem /build/cpp/orc_ep-install/include -isystem /opt/vcpkg/installed/amd64-linux-static-release/share/rapidjson/../../include -isystem /build/cpp/jemalloc_ep-prefix/src -Wno-noexcept-type -Wno-subobject-linkage  -fdiagnostics-color=always  -Wall -fno-semantic-interposition -msse4.2  -O3 -DNDEBUG -O2 -ftree-vectorize  -fPIC -pthread -DS2N_KYBER512R3_AVX2_BMI2 -DS2N_STACKTRACE -DS2N_CPUID_AVAILABLE -DS2N_FEATURES_AVAILABLE -fPIC -DS2N_FALL_THROUGH_SUPPORTED -DS2N___RESTRICT__SUPPORTED -DS2N_MADVISE_SUPPORTED -DS2N_CLONE_SUPPORTED -std=c++17 -MD -MT src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx.o -MF src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx.o.d -o src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx.o -c /build/cpp/src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx
In file included from /build/cpp/src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx:15:
/arrow/cpp/src/arrow/ipc/dictionary.cc:40:8: error: redefinition of ‘struct std::hash<arrow::FieldPath>’
   40 | struct hash<arrow::FieldPath> {
      |        ^~~~~~~~~~~~~~~~~~~~~~
In file included from /build/cpp/src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_23_cxx.cxx:3:
/arrow/cpp/src/arrow/compute/kernels/vector_sort.cc:24:13: note: previous definition of ‘struct std::hash<arrow::FieldPath>’
   24 | struct std::hash<arrow::FieldPath> : public arrow::FieldPath::Hash {};
      |             ^~~~~~~~~~~~~~~~~~~~~~

Job: https://github.com/apache/arrow/actions/runs/5337702550/jobs/9674081817 (zip)

Component(s)

C++

@benibus benibus self-assigned this Jun 21, 2023
@pitrou pitrou added this to the 13.0.0 milestone Jun 22, 2023
pitrou pushed a commit that referenced this issue Jun 22, 2023
… possible (#36222)

### Rationale for this change

Internal specializations of `std::hash<FieldPath>` have caused multiple-definition errors with unity builds.

### What changes are included in this PR?

To avoid exposing a global specialization of `std::hash` (which must be declared in the `std` namespace), this specifies the `FieldPath::Hash` helper as a template parameter to hashable containers instead.

### Are these changes tested?

Yes (covered by existing tests)

### Are there any user-facing changes?

No

* Closes: #36214

Authored-by: benibus <bpharks@gmx.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants