util: introduce and use ARRAY_SIZE macro

[theStack]

This PR is another tiny improvement found while working on #1765, with the goal to avoid code repetition.

The ARRAY_SIZE macro definition is pretty wide-spread in C projects and e.g. matches the one used in the Linux Kernel (without the additional check to reject pointers, as we would need GNU C for that, see e.g. https://stackoverflow.com/a/19455169; not sure if a useful counterpart exists that only relies on C89). Replacement instances were identified via $ git grep sizeof.*/.*sizeof.

[kevkevinpal]

I think overall the ARRAY_SIZE macro makes the codebase overall cleaner.

I'm just unsure because we can pass pointers into ARRAY_SIZE and have it silently pass.

I would add a comment above the macro to say to avoid using pointers, so if someone were to use the macro, they would be aware.

[w0xlt]

ACK 921b971

[real-or-random]

(without the additional check to reject pointers, as we would need GNU C for that, see e.g. stackoverflow.com/a/19455169;

GNU C is not an issue if we sandwich it in a #if SECP256K1_GNUC_PREREQ. A check that works only on GCC will be enough for us; we'll always test on GCC.

And we don't need BUILD_BUG_ON_ZERO because we have a similar STATIC_ASSERT macro. (Though I really like the implementation of BUILD_BUG_ON_ZERO and that it is an expression and not a statement; our macro is a statement that cannot be used anywhere BUILD_BUG_ON_ZERO can be used.)

[real-or-random]

Concept ACK

[theStack]

(without the additional check to reject pointers, as we would need GNU C for that, see e.g. stackoverflow.com/a/19455169;

GNU C is not an issue if we sandwich it in a #if SECP256K1_GNUC_PREREQ. A check that works only on GCC will be enough for us; we'll always test on GCC.

Ah good point, didn't consider that. I found that the needed typeof keyword is only available if the GNU extensions are explicitly enabled via e.g. -std=gnu90 (as opposed to -std=c90 what we use) though, but the alternative keyword __typeof__ seems to work.

And we don't need BUILD_BUG_ON_ZERO because we have a similar STATIC_ASSERT macro. (Though I really like the implementation of BUILD_BUG_ON_ZERO and that it is an expression and not a statement; our macro is a statement that cannot be used anywhere BUILD_BUG_ON_ZERO can be used.)

Isn't it being an expression necessary for most our use cases? We primarily take use of it in the condition of for loops (i.e. for (i = 0; i < ARRAY_SIZE(...); i++), I assume STATIC_ASSERT wouldn't work there.

Will take a deeper look soon and put the PR into draft state in any case until we figure out how to best add a "reject pointers" check.

[real-or-random]

Isn't it being an expression necessary for most our use cases? We primarily take use of it in the condition of for loops (i.e. for (i = 0; i < ARRAY_SIZE(...); i++), I assume STATIC_ASSERT wouldn't work there.

Good point. Then we'll need to port the macro.

Or perhaps we should just drop -std=c90, which would give us _Static_assert and make some of the C hacks obsolete (see https://github.com/torvalds/linux/blob/75a452d31ba697fc986609dd4905294e07687992/include/linux/compiler.h#L203). We could do a preprocessor check on __STDC_VERSION__ to determine if we have _Static_assert.

The reason we pass -std=c90 is to make sure that we only use C90 features. We could drop it and add a single CI job to test compilation with -std=c90 for that purpose.

[theStack]

Or perhaps we should just drop -std=c90, which would give us _Static_assert and make some of the C hacks obsolete (see https://github.com/torvalds/linux/blob/75a452d31ba697fc986609dd4905294e07687992/include/linux/compiler.h#L203). We could do a preprocessor check on STDC_VERSION to determine if we have _Static_assert.

The following works in my environment with gcc 14.2.0, after bumping the standard to C11 (needed to get _Static_assert) and removing -pedantic (needed to avoid a struct has no members [-Wpedantic] warning in the BUILD_BUG_ON_ZERO macro): theStack@d02a5de

Can be tested by e.g.

--- a/src/modules/schnorrsig/bench_impl.h
+++ b/src/modules/schnorrsig/bench_impl.h
@@ -52,6 +52,7 @@ static void run_schnorrsig_bench(int iters, int argc, char** argv) {

     data.ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
     data.keypairs = malloc(iters * sizeof(secp256k1_keypair *));
+    i = ARRAY_SIZE(data.keypairs); /* this should fail */
     data.pk = malloc(iters * sizeof(unsigned char *));
     data.msgs = malloc(iters * sizeof(unsigned char *));
     data.sigs = malloc(iters * sizeof(unsigned char *));

It seems though that even without the must_be_array addition, the compiler is able to detect misusage of ARRAY_SIZE, and there is a dedicated warning category (-Wsizeof-pointer-div):

/home/thestack/secp256k1/src/util.h:191:39: warning: division ‘sizeof (const secp256k1_keypair **) / sizeof (const secp256k1_keypair *)’ does not compute the number of array elements [-Wsizeof-pointer-div]
  191 | # define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
      |                                       ^
/home/thestack/secp256k1/src/modules/schnorrsig/bench_impl.h:55:9: note: in expansion of macro ‘ARRAY_SIZE’
   55 |     i = ARRAY_SIZE(data.keypairs); /* this should fail */
      |         ^~~~~~~~~~

[real-or-random]

after bumping the standard to C11 (needed to get _Static_assert)

What if you don't pass -std at all?

And removing -pedantic (needed to avoid a struct has no members [-Wpedantic] warning in the BUILD_BUG_ON_ZERO macro): theStack@d02a5de

I guess you could just add a member.

[theStack]

after bumping the standard to C11 (needed to get _Static_assert)

What if you don't pass -std at all?

And removing -pedantic (needed to avoid a struct has no members [-Wpedantic] warning in the BUILD_BUG_ON_ZERO macro): theStack@d02a5de

I guess you could just add a member.

@real-or-random: Good points. Removed the explicit setting of a C standard version (for some reason I assumed that setting CMAKE_C_STANDARD is mandatory in CMake, but apparently it isn't) and added a dummy member in the struct, so the -pedantic flag can be kept: theStack@62947de

I wonder if its worth it implementing this? Seems to be quite some code for something that can be detected by modern compilers anyways (though I didn't look into the details of -Wsizeof-pointer-div, maybe it doesn't catch as much).

[real-or-random]

I wonder if its worth it implementing this? Seems to be quite some code for something that can be detected by modern compilers anyways (though I didn't look into the details of -Wsizeof-pointer-div, maybe it doesn't catch as much).

Oh, I wasn't aware of the warning at all. Seems to work: https://godbolt.org/z/vxehGPznG

Yes, then the warning is better... I mean, there could be specific circumstances under which this won't work, but this would be rather strange because the expression will always look the same if generated by the macro.

[real-or-random]

utACK 921b971

[real-or-random]

@theStack Are you okay with this conclusion? And, if yes, is the PR ready from your side? (Sorry, I shouldn't have marked it ready for you.)

[theStack]

@theStack Are you okay with this conclusion? And, if yes, is the PR ready from your side? (Sorry, I shouldn't have marked it ready for you.)

Yes, I agree with the conclusion and think the PR is ready 👌 Thanks for verifying on godbolt! (and no worries, I think unmarking draft state was fine)