test: enable -Wunused-function in test suite (Fix #1831) #1865

This PR addresses issue #1831 by enabling the -Wunused-function compiler warning within the test suite.

Currently, -Wno-unused-function is passed globally to disable warnings about unused functions, making it too easy to write a test case but forget to actually call it. To catch untested helper functions safely, this PR uses GCC/Clang pragmas scoped strictly to the body of the test files.

Changes Made:

• Added #pragma GCC diagnostic warning "-Wunused-function" directly after the #include statements in src/tests.c, src/tests_exhaustive.c, src/ctime_tests.c, and src/unit_test.c. Fixes #1831

Thanks for the PR. :)

Concept ACK

Thanks for the quick update.

ACK mod that this should be squashed

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

We follow the same workflow as Bitcoin Core here, see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits for background.

@hebasto Want to review this?

~From #1831#issue-4010176205:~ ~> ... we should enable -Wunused-function at least in the test suite.~

~Could this be enabled as well?~

Concept ACK.

An alternative approach would be appending -Wunused-function to the compiler flags when building test targets. However, that would require changing and then maintaining for both build systems.

From #1831#issue-4010176205:

It's too easy to add a test but forget to call it.

Tested this scenario. Works as expected.

@kallal79

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

Just checking: Did you see this request? (I saw that you gave a few thumbs-ups in later comment.)

@kallal79

Could you squash the 2nd and 3rd commit into the 1st one and force-push?

Just checking: Did you see this request? (I saw that you gave a few thumbs-ups in later comment.)

And #1865 (review)?

@kallal79 Thanks for the update! I believe you introduced a mistake when squashing the commits. Now there's only a single commits beyond the desired changes, it also reverts some recent commits on master.

Tested a77dacad9ad8a2bff51b51c6a000c7dae45dad78:

$ gcc --version
gcc (Debian 4.3.2-1.1) 4.3.2
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc -O2 -c src/precomputed_*.c
$ gcc -O2 -o noverify_tests src/tests.c precomputed_ecmult.o precomputed_ecmult_gen.o
In file included from src/tests.c:29:
src/unit_test.c:21: warning: expected [error|warning|ignored] after '#pragma GCC diagnostic'
src/unit_test.c:487: warning: expected [error|warning|ignored] after '#pragma GCC diagnostic'
src/tests.c:41: warning: expected [error|warning|ignored] after '#pragma GCC diagnostic'
src/tests.c:8101: warning: expected [error|warning|ignored] after '#pragma GCC diagnostic'
src/util.h:34: warning: 'print_buf_plain' defined but not used
src/assumptions.h:27: warning: 'secp256k1_assumption_checker' defined but not used
src/hash_impl.h:251: warning: 'secp256k1_hmac_sha256_clear' defined but not used
src/testrand_impl.h:121: warning: 'testrand_flip' defined but not used

ACK a77dacad9ad8a2bff51b51c6a000c7dae45dad78

Tested a77daca:

I took the liberty to reset this to this commit, removing https://github.com/bitcoin-core/secp256k1/pull/1865/commits/2999a607f2539dd182c6f50ac15a43847ec23659 and its revert https://github.com/bitcoin-core/secp256k1/pull/1865/commits/6f77415f101425f85be67b3ef57f81ad85eeb497.

I believe the test results with gcc 4.3.2 are entirely fine. Warnings are acceptable if you use such an old compiler. @hebasto Are you saying they're not fine?

ACK a77daca

Tested a77daca:

I took the liberty to reset this to this commit, removing 2999a60 and its revert 6f77415.

I believe the test results with gcc 4.3.2 are entirely fine. Warnings are acceptable if you use such an old compiler. @hebasto Are you saying they're not fine?

Currently, when building with GCC 4.2 through 4.5.x, the -Wunused-function diagnostic isn't limited to the designated sources (since push/pop wasn't introduced until 4.6).

GCC versions older than 4.2 simply drop unrecognized #pragmas without failing:

$ gcc-4.1 -O2 src/tests.c src/precomputed_*.c -o noverify_tests

However, if explicitly enabled with -Wunknown-pragmas, they surface as expected:

$ gcc-4.1 -O2 -Wunknown-pragmas src/tests.c src/precomputed_*.c -o noverify_tests
In file included from src/tests.c:29:
src/unit_test.c:21: warning: ignoring #pragma GCC diagnostic
src/unit_test.c:22: warning: ignoring #pragma GCC diagnostic
src/unit_test.c:487: warning: ignoring #pragma GCC diagnostic
src/tests.c:41: warning: ignoring #pragma GCC diagnostic
src/tests.c:42: warning: ignoring #pragma GCC diagnostic
src/tests.c:8101: warning: ignoring #pragma GCC diagnostic

ACK a77dacad9ad8a2bff51b51c6a000c7dae45dad78.

These changes are useful for our current development. Just note that a bit more care is needed when using GCC older than 4.6, but I think that's fine.

Apparently, the push/pop machinery does not work as expected when using GCC 4.7.2:

src/tests.c: At top level:
src/util.h:34:13: warning: 'print_buf_plain' defined but not used [-Wunused-function]

Hm, and on GCC 5, it won't warn at all (which is also fine). It seems to work properly from GCC 6. Here's a godbolt link to quickly check this on different compilers. https://godbolt.org/z/co97Pbbe6

Anyway, I think this is still fine. If someone really wants to use GCC 4, then they need to accept that things may be not optimal.

Hm, and on GCC 5, it won't warn at all (which is also fine). It seems to work properly from GCC 6.

Documenting GCC 10.2, shipped in Debian "oldoldstable", as the minimum recommended compiler should address this issue.

Thanks for the reviews and merge — I’ll keep the GCC diagnostic push/pop nuances in mind for future contributions.