[libcxx-commits] [libcxx] [libc++] Fix bogus integer sanitizer warnings in hash helpers (PR #146715)

[Louis Dionne via libcxx-commits]

https://github.com/ldionne commented:

This makes sense to me. Just to make sure we're on the same page: the mathematical result of this left shift is indeed not representable in an `unsigned long`, but that's okay because we discard the bits that are shifted beyond the "end" of the integer. So we have:

```
__val << (64 - __shift)
```

where `__val = 17751278790833232267U` and `__shift = 43`, giving `17751278790833232267U << 21`. That number would be `37302001761581901365411840` which doesn't fit inside an `unsigned long`, but in reality what we get is just the lower 64 bits of that number, aka `5770268451330048`.

So the operation itself is valid and the UBSan warning is bogus. If I got that right, then I'm fine with this patch.

I do agree with @philnik777 that we should enable this in our sanitizer CI. This should be doable with something like this patch:

```diff
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index dffdd7a3c70a..9bbd64cce370 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -611,10 +611,10 @@ function(get_sanitizer_flags OUT_VAR  USE_SANITIZER)
         append_flags(SANITIZER_FLAGS "-fsanitize-memory-track-origins")
       endif()
     elseif (USE_SANITIZER STREQUAL "Undefined")
-      append_flags(SANITIZER_FLAGS "-fsanitize=undefined" "-fno-sanitize=vptr,function" "-fno-sanitize-recover=all")
+      append_flags(SANITIZER_FLAGS "-fsanitize=undefined,integer" "-fno-sanitize=vptr,function" "-fno-sanitize-recover=all")
     elseif (USE_SANITIZER STREQUAL "Address;Undefined" OR
             USE_SANITIZER STREQUAL "Undefined;Address")
-      append_flags(SANITIZER_FLAGS "-fsanitize=address,undefined" "-fno-sanitize=vptr,function" "-fno-sanitize-recover=all")
+      append_flags(SANITIZER_FLAGS "-fsanitize=address,undefined,integer" "-fno-sanitize=vptr,function" "-fno-sanitize-recover=all")
     elseif (USE_SANITIZER STREQUAL "Thread")
       append_flags(SANITIZER_FLAGS -fsanitize=thread)
     elseif (USE_SANITIZER STREQUAL "DataFlow")
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index fc34009d0a55..65715117e7dc 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -317,8 +317,8 @@ DEFAULT_PARAMETERS = [
             [
                 AddFlag("-g -fno-omit-frame-pointer") if sanitizer else None,
 
-                AddFlag("-fsanitize=undefined -fno-sanitize=float-divide-by-zero -fno-sanitize-recover=all") if sanitizer == "Undefined" else None,
-                AddFeature("ubsan")                                                                          if sanitizer == "Undefined" else None,
+                AddFlag("-fsanitize=undefined,integer -fno-sanitize=float-divide-by-zero -fno-sanitize-recover=all") if sanitizer == "Undefined" else None,
+                AddFeature("ubsan")                                                                                  if sanitizer == "Undefined" else None,
 
                 AddFlag("-fsanitize=address") if sanitizer == "Address" else None,
                 AddFeature("asan")            if sanitizer == "Address" else None,
```

That will bundle `-fsanitize=integer` into our existing ubsan checks, but I think that should be reasonable?

https://github.com/llvm/llvm-project/pull/146715