In Bitcoin Core, the “MSan” CI jobs use the latest tagged Clang available from http://apt.llvm.org.
This PR applies similar changes and switches the “MSan” CI jobs to clang-snapshot.
This exposes problematic code that was reported in https://github.com/bitcoin/bitcoin/issues/33284.
From https://github.com/bitcoin-core/secp256k1/actions/runs/17829295048/job/50691626826:
0 CC src/noverify_tests-tests.o
1src/tests.c:6051:34: error: variable 'pubkey' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
2 6051 | SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
3 | ^~~~~~
41 error generated.
From https://github.com/bitcoin-core/secp256k1/actions/runs/17829295048/job/50691626826:
0 CC src/noverify_tests-tests.o 1src/tests.c:6051:34: error: variable 'pubkey' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer] 2 6051 | SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey)); 3 | ^~~~~~ 41 error generated.
Added a commit to silence the -Wuninitialized-const-pointer warning.
I think that’s a Clang bug.
SECP256K1_CHECKMEM_UNDEFINE expands to __msan_allocated_memory with signature:
0/* Tell MSan about newly allocated memory (ex.: custom allocator).
1 Memory will be marked uninitialized, with origin at the call site. */
2void SANITIZER_CDECL __msan_allocated_memory(const volatile void *data,
3 size_t size);
So yes, we’re passing a const pointer to an uninitialized portion of memory, which is typically pointless: the callee can’t read it (UB) and it is not allowed to write it either.
But __msan_allocated_memory is special in this regard. The call is meaningful. And since it’s a part of the Clang runtime, Clang should understand that this is meaningful.
Are you willing to report this upstream?
On our side, I think there are some nicer options:
pubkey is already uninitialized). But it’s easy to overlook in the future.I tend to 3. It’s perhaps the most complex option, but also the most future-proof?
I think that’s a Clang bug.
I agree with your analysis. That’s why I chose to suppress the warning rather than take another approach.
- Wrap the suppression inside the macro to make sure we always avoid this bug.
I tend to 3. It’s perhaps the most complex option, but also the most future-proof?
That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?
That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?
Can you try this in checkmem.h?
0#if defined(__has_feature)
1# if __has_feature(memory_sanitizer)
2# include <sanitizer/msan_interface.h>
3# define SECP256K1_CHECKMEM_ENABLED 1
4# if defined(__clang__)
5# define SECP256K1_CHECKMEM_UNDEFINE(p, len) do { \
6 _Pragma("clang diagnostic push") \
7 _Pragma("clang diagnostic ignored \"-Wuninitialized-const-pointer\"") \
8 __msan_allocated_memory((p), (len)); \
9 _Pragma("clang diagnostic pop") \
10 } while(0)
11# else
12# define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
13# endif
14# define SECP256K1_CHECKMEM_DEFINE(p, len) __msan_unpoison((p), (len))
15# define SECP256K1_CHECKMEM_MSAN_DEFINE(p, len) __msan_unpoison((p), (len))
16# define SECP256K1_CHECKMEM_CHECK(p, len) __msan_check_mem_is_initialized((p), (len))
17# define SECP256K1_CHECKMEM_RUNNING() (1)
18# endif
19#endif
I think that’s a Clang bug.
I agree with your analysis. That’s why I chose to suppress the warning rather than take another approach.
Would you be willing to report it upstream? Perhaps it will be fixed before the release of clang 21 and then we’ll be able to drop this workaround.
That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?
Can you try this in
checkmem.h?
Thanks! Taken.
UPD. _Pragma(...) is a C99 feature.
Would you be willing to report it upstream?
https://github.com/llvm/llvm-project/issues/160094.
Perhaps it will be fixed before the release of clang 21 and then we’ll be able to drop this workaround.
https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.1
UPD.
_Pragma(...)is a C99 feature.
Indeed. If I read correctly, clang has always supported it. https://clang.llvm.org/c_status.html
47@@ -48,7 +48,16 @@
48 # if __has_feature(memory_sanitizer)
49 # include <sanitizer/msan_interface.h>
50 # define SECP256K1_CHECKMEM_ENABLED 1
51-# define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
52+# if defined(__clang__)
53+# define SECP256K1_CHECKMEM_UNDEFINE(p, len) do { \
54+ _Pragma("clang diagnostic push") \
0 # Work around https://github.com/llvm/llvm-project/issues/160094
1 _Pragma("clang diagnostic push") \
Co-authored-by: Tim Ruffing <me@real-or-random.org>