ORC-1841: [C++][CI] Add UBSAN support to Cmake #2117
Conversation
.github/workflows/ubsan_test.yml
Outdated
| CXX: ${{ matrix.cxx }} | ||
| run: | | ||
| mkdir build && cd build | ||
| cmake .. -DCMAKE_BUILD_TYPE=Debug -DENABLE_UBSAN=ON -DBUILD_JAVA=OFF |
There was a problem hiding this comment.
Can we enable ASan and UBSan in the same workflow?
.github/workflows/ubsan_test.yml
Outdated
| CXX: ${{ matrix.cxx }} | ||
| run: | | ||
| mkdir build && cd build | ||
| cmake .. -DCMAKE_BUILD_TYPE=Debug -DENABLE_UBSAN=ON -DBUILD_JAVA=OFF |
There was a problem hiding this comment.
Maybe we should turn on following options:
-DBUILD_ENABLE_AVX512=ON -DBUILD_CPP_ENABLE_METRICS=ON
There was a problem hiding this comment.
Why do we need to collect metrics in a CI workflow?
There was a problem hiding this comment.
Why do we need to collect metrics in a CI workflow?
There are some codes only enabled when the macroENABLE_METRICS is defined, we need to check them too.
|
@luffy-zh Thank you. I did notice this PR and created a similar one here: #2267 Can we move forward with UBSAN support? |
| #endif | ||
|
|
||
| #if defined(__GNUC__) || defined(__clang__) | ||
| #define NO_SANITIZE_ATTR __attribute__((no_sanitize("signed-integer-overflow", "shift"))) |
There was a problem hiding this comment.
Is it possible to fix the code where signed-integer-overflow and shift issues were discovered?
Is it temporary?
There was a problem hiding this comment.
It's a temporary workaround. I intend to split the necessary changes into multiple patches to fix the issue discovered by UBSAN.
|
@ffacs can we merge this PR? |
|
@wgtmac can we merge this PR? |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @luffy-zh , @wgtmac , @ffacs , @sfc-gh-mkwiczala .
|
Merged to main for Apache ORC 2.2.0. |
…identified by UBSAN ### What changes were proposed in this pull request? Remove some temporary workarounds in this [PR](#2117). ### Why are the changes needed? Fix potential overflow issue in left shift operation. ### How was this patch tested? Test it locally. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #2317 from luffy-zh/ORC-1946. Authored-by: luffy-zh <zhnice@outlook.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…identified by UBSAN ### What changes were proposed in this pull request? Remove some temporary workarounds in this [PR](#2117). ### Why are the changes needed? Fix potential overflow issue in left shift operation. ### How was this patch tested? Test it locally. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #2317 from luffy-zh/ORC-1946. Authored-by: luffy-zh <zhnice@outlook.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit eb60462) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
5 participants
What changes were proposed in this pull request?
Add support for UBASN in CMake options and enable it by default in the CI workflow.
Why are the changes needed?
Detect undefined behavior issues with the help of the undefined sanitizer.
How was this patch tested?
Pass all CIs.
Was this patch authored or co-authored using generative AI tooling?
NO.