Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hebasto opened this issue Dec 2, 2021 · 6 comments · Fixed by #1027
Closed

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

hebasto opened this issue Dec 2, 2021 · 6 comments · Fixed by #1027

Comments

@hebasto
Copy link
Member

hebasto commented Dec 2, 2021

Steps to reproduce on master (fecf436):

$ ./configure --host=riscv64-linux-gnu
$ make clean
$ $ make
  CC       src/bench.o
gcc -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
gcc -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
./gen_context
  CC       src/libsecp256k1_la-secp256k1.lo
src/secp256k1.c: In function ‘secp256k1_declassify’:
src/secp256k1.c:227:93: warning: unused parameter ‘p’ [-Wunused-parameter]
  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
      |                                                                                 ~~~~~~~~~~~~^
src/secp256k1.c:227:103: warning: unused parameter ‘len’ [-Wunused-parameter]
  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
      |                                                                                                ~~~~~~~^~~
  CCLD     libsecp256k1.la
  CCLD     bench
  CC       src/bench_internal-bench_internal.o
  CCLD     bench_internal
  CC       src/bench_ecmult-bench_ecmult.o
  CCLD     bench_ecmult
  CC       src/tests-tests.o
In file included from src/tests.c:17:
src/secp256k1.c: In function ‘secp256k1_declassify’:
src/secp256k1.c:227:93: warning: unused parameter ‘p’ [-Wunused-parameter]
  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
      |                                                                                 ~~~~~~~~~~~~^
src/secp256k1.c:227:103: warning: unused parameter ‘len’ [-Wunused-parameter]
  227 | static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, const void *p, size_t len) {
      |                                                                                                ~~~~~~~^~~
  CCLD     tests
  CC       src/valgrind_ctime_test.o
  CCLD     valgrind_ctime_test
  CC       src/exhaustive_tests-tests_exhaustive.o
  CCLD     exhaustive_tests
@real-or-random
Copy link
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

@hebasto
Copy link
Member Author

hebasto commented Dec 2, 2021

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.

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

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

@real-or-random
Copy link
Contributor

real-or-random commented Dec 2, 2021

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.

@hebasto
Copy link
Member Author

hebasto commented Dec 2, 2021

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.

@hebasto
Copy link
Member Author

hebasto commented Dec 2, 2021

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?

@real-or-random
Copy link
Contributor

real-or-random commented Dec 3, 2021

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 (#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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants