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.
Nice. This fixes #1511 for me, under Clang 17 (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.
utACK 241a2145d15f5b9e2978c3a76d4fec9eefbc96f8
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
void 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]. */
/* 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]...
Ah, I figured comments like l6 still made sense conceptually, but sure, I'll update them to match.
void 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.
ACK f7f0184ba1358cb8659607d2d0105628cb130e4f tests work fine with clang 17 and ./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
utACK f7f0184ba1358cb8659607d2d0105628cb130e4f
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.