`std::rotr()` causes implicit-integer-sign-change UBSan failure if passed negative shift

[chfast]

The following program built with -std=c++23 -stdlib=libc++ -fsanitize=implicit-integer-sign-change
causes:

include/c++/v1/__bit/rotate.h:27:8: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/compiler-explorer/clang-18.1.0/bin/../include/c++/v1/__bit/rotate.h:27:8 

#include <bit>

int main()
{
    return std::rotr(0u, -1);
}

https://godbolt.org/z/EveTsa8Gs

If you think this is worth fixing I can submit a patch.

[llvmbot]

@llvm/issue-subscribers-bug

Author: Paweł Bylica (chfast)

The following program built with `-std=c++23 -stdlib=libc++ -fsanitize=implicit-integer-sign-change` causes:

include/c++/v1/__bit/rotate.h:27:8: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/compiler-explorer/clang-18.1.0/bin/../include/c++/v1/__bit/rotate.h:27:8 

#include <bit>

int main()
{
    return std::rotr(0u, -1);
}

If you think this is worth fixing I can submit a patch.

[chfast]

This is more severe because std::rotl is implemented in terms of std::rotr. Therefore the std::rotl(0u, 1) also causes the failure.

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __rotl(_Tp __t, int __cnt) _NOEXCEPT {
  return std::__rotr(__t, -__cnt);
}

https://github.com/llvm/llvm-project/blob/llvmorg-19-init/libcxx/include/__bit/rotate.h#L38-L41

[mordante]

Thanks for your report. I've created a fix.

[mk-huawei]

I believe there's a different issue here. Libc++ is built with sanitizers (UBSan as well), however, -fsanitize=undefined (as used in https://github.com/llvm/llvm-project/blob/main/libcxx/cmake/caches/Generic-ubsan.cmake), does not include -fsanitize=implicit-integer-sign-change. It is included however by -fsanitize=integer.
So, it would be maybe a good idea to add Generic-ubsan-integer.cmake cache and run it as part of the CI?
I guess it would not be green immediately, but it is probably better to add this before merging your fix (to be sure to see it fail).

[mordante]

According to https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks the flag is enabled by default in UBSAN. Also the tests for rotate were tested during constant evaluation, which means the original tests did not exhibit UB. Adding a test with INT_MIN as shift fails to compile due to UB with the original code.

[mk-huawei]

The thing is that this documentation is actually not correct about the included checks (it should exclude integer group).

You can compare the original author's snippet https://godbolt.org/z/EveTsa8Gs, with -fsanitize=implicit-integer-sign-change, it provokes sanitizer error.
With -fsanitize=undefined, the error is not provoked, cf. https://godbolt.org/z/a7jaKzWdz.
With -fsanitize=integer, the error is there again: https://godbolt.org/z/ec4boh7Me.

Also, from documentation (emphasis mine):

-fsanitize=implicit-integer-sign-change: Implicit conversion between integer types, if that changes the sign of the value. That is, if the original value was negative and the new value is positive (or zero), or the original value was positive, and the new value is negative. Issues caught by this sanitizer are not undefined behavior, but are often unintentional.

That's why there was no UB in constexpr context, because it's technically not a UB.

[mordante]

I see. I checked the code and the documentation indeed seems wrong. I'm running a test with libc++ and implicit-integer-sign-change.