This was missed in 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 because older versions of clang did not complain about it. But clang-17, at least, does.
The array-as-a-param makes this annoying because sizeof(l) is not helpful. I’d be happy to change the size calculation if there are any better suggestions or strong preferences.
Ubuntu clang version 17.0.6 (5build1)) and 18 (Ubuntu clang version 18.1.0 (rc2-4)). I’ve also cherry-picked it into https://github.com/bitcoin/bitcoin/pull/29742 to run through our CI.
The array-as-a-param makes this annoying because
sizeof(l)is not helpful.
Hm, yeah, idk, the uint64_t l[8] parameter declaration is a bit strange, or at least unusual for this code base. I’d ACK a patch that changes this a pointer declaration, which we usually do in the codebase. If you do, maybe also rename to l8.
For others: The issue is that, despite the declaration, l will still decay to a pointer, i.e., l won’t be of array type, but of type uint64_t*. And the [8] has no significance. That means that sizeof(l) == sizeof(uint64_t*), which, for maximum confusion, happens to be 8 coincidentally.
TIL of the weird-looking
0void func(uint64_t l[static 8])
syntax in c99: https://stackoverflow.com/questions/3430315/what-is-the-purpose-of-static-keyword-in-array-parameter-of-function-like-char
Sadly it doesn’t do anything to help us here, just thought I’d share. Yet another totally unrelated thing that static can mean in c/c++ code :)
I’ll add a commit to change this to a pointer as suggested.
821 #else
822 /* 160 bit accumulator. */
823 uint64_t c0 = 0, c1 = 0;
824 uint32_t c2 = 0;
825
826 /* l[0..7] = a[0..3] * b[0..3]. */
0 /* l8[0..7] = a[0..3] * b[0..3]. */
And there are some other comments that may require updating… E.g., Extract l7. I admit that these are confusing now that the variable name is l8. But I guess it should anyway be Extract l8[7]…
l6 still made sense conceptually, but sure, I’ll update them to match.
0void func(uint64_t l[static 8])
Haha okay, I totally wasn’t aware of this, even though I happen to look at the standards text from time to time… And yeah, static is a bit cursed.
The behavior is identical, but the former syntax suggests guarantees that
don't actually exist.
This was missed in 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 because older
versions of clang did not complain about it. But clang-17, at least, does.
./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
For posterity, the reason why clang 15 was happy with just the annotation in https://github.com/bitcoin-core/secp256k1/commit/31ba40494428dcbf2eb5eb6f2328eca91b0b0746 is this:
The default of what is considered a “use” of uninitialized memory was changed in clang 16. Returning an uninitialized variables from a function, or passing uninitialized values to a function as a parameter is now considered, and MSan will report it by default. See the Clang 16.0.09 Release Notes:
-fsanitize-memory-param-retvalis turned on by default. With-fsanitize=memory, passing uninitialized variables to functions and returning uninitialized variables from functions is more aggressively reported.-fno-sanitize-memory-param-retvalrestores the previous behavior.