-Wunused-parameter warnings when cross-compiling for riscv64-linux-gnu #1023

issue hebasto openend this issue on December 2, 2021
  1. hebasto commented at 5:16 pm on December 2, 2021: member

    Steps to reproduce on master (fecf436d5327717801da84beb3066f5a9b80ea8e):

     0$ ./configure --host=riscv64-linux-gnu
     1$ make clean
     2$ $ make
     3  CC       src/bench.o
     4gcc -DHAVE_CONFIG_H -I. -I./src -O2  -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -fvisibility=hidden -g -O2 -c src/gen_context.c -o gen_context.o
     5gcc -O2  -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -fvisibility=hidden -g -O2  gen_context.o -o gen_context
     6./gen_context
     7  CC       src/libsecp256k1_la-secp256k1.lo
     8src/secp256k1.c: In function secp256k1_declassify:
     9src/secp256k1.c:227:93: warning: unused parameter p [-Wunused-parameter]
    10  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
    11      |                                                                                 ~~~~~~~~~~~~^
    12src/secp256k1.c:227:103: warning: unused parameter len [-Wunused-parameter]
    13  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
    14      |                                                                                                ~~~~~~~^~~
    15  CCLD     libsecp256k1.la
    16  CCLD     bench
    17  CC       src/bench_internal-bench_internal.o
    18  CCLD     bench_internal
    19  CC       src/bench_ecmult-bench_ecmult.o
    20  CCLD     bench_ecmult
    21  CC       src/tests-tests.o
    22In file included from src/tests.c:17:
    23src/secp256k1.c: In function secp256k1_declassify:
    24src/secp256k1.c:227:93: warning: unused parameter p [-Wunused-parameter]
    25  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
    26      |                                                                                 ~~~~~~~~~~~~^
    27src/secp256k1.c:227:103: warning: unused parameter len [-Wunused-parameter]
    28  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
    29      |                                                                                                ~~~~~~~^~~
    30  CCLD     tests
    31  CC       src/valgrind_ctime_test.o
    32  CCLD     valgrind_ctime_test
    33  CC       src/exhaustive_tests-tests_exhaustive.o
    34  CCLD     exhaustive_tests
    
  2. real-or-random commented at 5:57 pm on December 2, 2021: contributor

    @hebasto Is valgrind support enabled? See output of configure. If yes, I wonder what the definition of VALGRIND_MAKE_MEM_DEFINED in valgrind.h is.

    see https://github.com/bitcoin-core/secp256k1/blob/master/src/secp256k1.c#L229

  3. hebasto commented at 6:10 pm on December 2, 2021: member

    Is valgrind support enabled? See output of configure.

    Yes.

    If yes, I wonder what the definition of VALGRIND_MAKE_MEM_DEFINED in valgrind.h is.

    0$ grep /usr/include/valgrind/valgrind.h -n -e VALGRIND_MAKE_MEM
    16459:   writes to freed blocks in any way then a VALGRIND_MAKE_MEM_UNDEFINED call
    26465:   data structures, VALGRIND_MAKE_MEM_UNDEFINED calls will also be necessary.
    

    i.e., VALGRIND_MAKE_MEM_DEFINED is not defined, right?

  4. real-or-random commented at 7:26 pm on December 2, 2021: contributor

    Hm, well, it’s defined in memcheck.h in terms of another macro which is defined in valgrind.h again, depending on the architecture. It’s probably empty for riscv64 because I think the issue is just that valgrind does not have support for riscv64: https://sourceware.org/git/?p=valgrind.git;a=blob;f=README;h=ae21cc74d6eced4e632bc630690d907bdadb9aba;hb=HEAD (unless in some experimental fork: https://github.com/petrpavlu/valgrind-riscv64 )…

    Hm I don’t know. We could suppress the warning but it would be nice if we could output have a more meaningful warning, or let configure detect that anyway valgrind support won’t work.

    I don’t think it’s a big issue in the end, running the valgrind constant time test on valgrind should then anyway not possible, so at least you cannot wrongly conclude that the test passes.

  5. hebasto commented at 7:30 pm on December 2, 2021: member

    We could suppress the warning but it would be nice if we could output have a more meaningful warning, or let configure detect that anyway valgrind support won’t work.

    The latter looks like a better approach for me.

  6. hebasto commented at 9:55 pm on December 2, 2021: member

    I don’t think it’s a big issue in the end, running the valgrind constant time test on valgrind should then anyway not possible, so at least you cannot wrongly conclude that the test passes.

    I’m confused. In cross-compiling setup the valgrind_ctime_test is compiled for a host platform, therefore, there is no sense to run it on a build platform. So what is the point to enable valgrind when cross compiling?

    Are all of the tests supposed to be delivered to a host system within a “package” with libsecp256k1.so?

  7. real-or-random commented at 10:02 am on December 3, 2021: contributor

    I’m confused. In cross-compiling setup the valgrind_ctime_test is compiled for a host platform, therefore, there is no sense to run it on a build platform. So what is the point to enable valgrind when cross compiling?

    Are all of the tests supposed to be delivered to a host system within a “package” with libsecp256k1.so?

    Whether people really do this in practice is a different question but sure, ideally you run the tests on the host platform where you also run the library. Maybe I’m stating the obvious here but the purpose of the tests is not only to catch bugs in the code but also bugs introduced by the compiler or bugs that occur only in a specific execution environment.

    And valgrind_ctime_test is somewhat special in this sense. It detects non-constant time behavior and this kind of behavior is by definition inserted by the compiler, because programming languages typically don’t guarantee anything about the running time of the code. Moreover, optimizations are pretty specific to the host, and indeed we’ve seen compilers outputting non-constant time code in some cases on only some platforms with some compiler options (https://github.com/bitcoin-core/secp256k1/issues/784, #771, #708 (comment)). So valgrind_ctime_test really only makes sense on the host.

    edit: Of course it would be good to run this at least on all platforms that Core officially supports…

  8. hebasto cross-referenced this on Dec 3, 2021 from issue build: Add a check that Valgrind actually supports a host platform by hebasto
  9. real-or-random closed this on Dec 5, 2021


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me