Fix undefined behavior in is_zero_byte

[masterleinad]

#include <Kokkos_Core.hpp>
 
int main(int argc, char* argv[]) {
  Kokkos::ScopeGuard scope_guard;
  Kokkos::View<long double, Kokkos::OpenMP> dummy("C");
}

would fail with icpx and -fp-model=precise -O2 -g -DNDEBUG -fiopenmp due to the type punning approach used in is_zero_byte. This also manifests in the test suite with openmp.numeric_traits_infinity segfaulting.

Using bitcast (which requires using Kokkos::Array in turn) instead fixes the issue. An alternative is to always use char as a comparison type but I went for minimal modifications.

[dalg24]

Did the UB or address sanitizer report anything?

[dalg24]

I am not a big fan of the header include comments. I find them confusing as usually we will comment what from that header include we are using and here you reversed the dependency. I would prefer if you drop both comments.

[dalg24]

Would you remind me if we had a good reason to use that common type rather than just an array of unsigned char or whatnot?

[masterleinad]

Would you remind me if we had a good reason to use that common type rather than just an array of unsigned char or whatnot?

We just thought it was faster to compare bigger chunks of memory but the function is not really performance-sensitive in its current use.

[dalg24]

Would you remind me if we had a good reason to use that common type rather than just an array of unsigned char or whatnot?

We just thought it was faster to compare bigger chunks of memory but the function is not really performance-sensitive in its current use.

I think if we had done std::array<std::byte, sizeof(T)> and loop over the bytes the the compiler can figure out that the loop bounds are known at compile time and optimize accordingly.
Anyway not asking you to change here.

[crtrott]

Blocking for a second: would replacing sizeof with alignof also fix the problem?

I am trying to figure out why this was segfaulting, and now it wouldn't. The only thing I could think of is that you check is_zero_byte for something like a pair<int,int> which is not 8-byte aligned. When simply recasting the reference to long long could result in a an unaligned memory access for an 8 byte value, while bit casting to Kokkos::Array<> is ok because it returns a copy?

[masterleinad]

Blocking for a second: would replacing sizeof with alignof also fix the problem?

No, the two are the same for these types.

I am trying to figure out why this was segfaulting, and now it wouldn't. The only thing I could think of is that you check is_zero_byte for something like a pair<int,int> which is not 8-byte aligned. When simply recasting the reference to long long could result in a an unaligned memory access for an 8 byte value, while bit casting to Kokkos::Array<> is ok because it returns a copy?

I mean bit casting just avoids undefined behavior when reinterpreting the memory pointed to. It's not clear to me what instruction the compiler generates that leads to a segfault, though. For reference, the backtrace look somewhat like

#0  0x00000000011c5880 in ?? ()
#1  0x00007fffecde6f69 in _Unwind_ForcedUnwind_Phase2 (exc=0x7fffffff6880, context=0x7fffffff6740, frames_p=0x7fffffff6648) at ../../../cpe-gcc-11.2.0-202108140355.9bf1fd589a5c1/libgcc/unwind.inc:170
#2  0x00007fffecde7905 in _Unwind_Resume (exc=0x7fffffff6880) at ../../../cpe-gcc-11.2.0-202108140355.9bf1fd589a5c1/libgcc/unwind.inc:243
#3  0x000000000040e368 in Kokkos::Impl::ViewValueFunctor<Kokkos::Device<Kokkos::OpenMP, Kokkos::HostSpace>, long double, true>::construct_shared_allocation<long double> (this=this@entry=0x55f100)
    at /opt/cray/pe/gcc/11.2.0/snos/lib/gcc/x86_64-suse-linux/11.2.0/../../../../include/g++/bits/basic_string.h:331
#4  0x000000000040de0a in Kokkos::Impl::ViewMapping<Kokkos::ViewTraits<long double, Kokkos::OpenMP>, void>::allocate_shared<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, Kokkos::HostSpace, Kokkos::OpenMP> (this=this@entry=0x7fffffff6ab0, arg_prop=..., arg_layout=..., execution_space_specified=<optimized out>) at /home/darndt/kokkos/core/src/impl/Kokkos_ViewMapping.hpp:3454
#5  0x000000000040d421 in Kokkos::View<long double, Kokkos::OpenMP>::View<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(Kokkos::Impl::ViewCtorProp<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const&, std::enable_if<!Impl::ViewCtorProp<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::has_pointer, Kokkos::LayoutRight>::type const&) (this=this@entry=0x7fffffff6aa8, arg_prop=..., arg_layout=...) at /home/darndt/kokkos/core/src/Kokkos_View.hpp:1421
#6  0x000000000040c70d in Kokkos::View<long double, Kokkos::OpenMP>::View<char [2]> (this=0x7fffffff6aa8, arg_N0=18446744073709551615, arg_N1=18446744073709551615, arg_N2=18446744073709551615, 
    arg_N3=18446744073709551615, arg_N4=18446744073709551615, arg_N5=18446744073709551615, arg_N6=18446744073709551615, arg_N7=18446744073709551615, arg_label=...)
    at /home/darndt/kokkos/core/src/Kokkos_View.hpp:1512
#7  TestNumericTraits<Kokkos::OpenMP, long double, Epsilon>::TestNumericTraits (this=0x7fffffff6a70) at /home/darndt/kokkos/core/unit_test/TestNumericTraits.hpp:90
[...]

[crtrott]

sizeof and alignof are not the same for types such as pair<int,int> but agreed they are apparently for long double which was the crash you saw there.

[ndellingwood]

@dalg24 does this need a bug fix changelog entry for 4.4? It looks like the issue was not newly introduced to develop after the most recent release?

[dalg24]

It was a longstanding issue. I suppose we should have an entry we got rid to UB in the logic to decide whether to do a zero-memset instead of launching a kernel at view initialization and in deep_copy with a scalar argument as the source (fill view with given value).
@masterleinad please add