GH-46123: [C++] Undefined behavior in compare_internal.cc and light_array_internal.cc

[jonkeane]

Rationale for this change

Removing undefined behaviors, adding a CI job for the M1-mac sanitizer now being run.

What changes are included in this PR?

The fixes + a sanitizer job that confirms they are fixed.

Are these changes tested?

Yup!

Are there any user-facing changes?

Fewer undefined behaviors.

• GitHub Issue: [C++] Undefined behavior in compare_internal.cc and light_array_internal.cc #46123

[github-actions]

⚠️ GitHub issue #46123 has been automatically assigned in GitHub to PR creator.

[jonkeane]

@github-actions crossbow submit test-r-m1-san

[github-actions]

Revision: 07d7582

Submitted crossbow builds: ursacomputing/crossbow @ actions-798e0f8cd9

Task	Status
test-r-m1-san

[zanmato1984]

LGTM.

Thank you @jonkeane for setting up the m1 sanitizer CI job. I think I can take good use of it to further figure out why the misalignment is originated and fixed.

[jonkeane]

@github-actions crossbow submit -g r

[github-actions]

Revision: 07d7582

Submitted crossbow builds: ursacomputing/crossbow @ actions-bccc2b44c0

Task	Status
r-binary-packages
r-recheck-most
test-r-arrow-backwards-compatibility
test-r-clang-asan
test-r-clang-ubsan
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-extra-packages
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-sanitizer
test-r-linux-valgrind
test-r-m1-san
test-r-macos-as-cran
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions

[zanmato1984]

OK, I did some experiments that can further justify this fix. This experiment proves that merely changing the pointer type passed into std::memcpy will affect how the compiler generates UB detection stub code which reports arguably false alarm (otherwise we can't explain why changing the pointer type from uint64_t * to uint8_t * (and passing into std::memcpy as void *) eliminates the sanitizer error).

For function (64-bit pointer)

int64_t read_mc64(const uint64_t* src, size_t n) {
    uint64_t result = 0;
    std::memcpy(&result, src, n);
    return result;
}

The compiler generates an alignment checking stub:

        tst     x0, #0x7
        b.ne    .LBB2_4

If the check failed (unaligned to 8-byte), it invokes UBSAN reporting routine __ubsan_handle_type_mismatch_v1 regardless of how it is used (in this function, passed into std::memcpy as a void *, which IMO is NOT UB).

Whereas for function (8-bit pointer)

uint64_t read_mc8(const uint8_t* src, size_t n) {
    uint64_t result = 0;
    std::memcpy(&result, src, n);
    return result;
}

No alignment checking stub so everything is fine.

Hope this could help people to understand better.

[jonkeane]

@assignUser any chance we could get this pulled into the 20 release branch? We still have enough runway with CRAN (5/10 is our deadline) that we might not need to do a special R-only release just for this if we do. Or at least, I think it's ok for us to wait and see if 20 makes it out before then.

[assignUser]

@jonkeane No problem, we need to do another RC anyway 👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c54b039.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[jonkeane]

There were no benchmark performance regressions. 🎉

I was a little worried this casting was going to hit us, but it looks like no!