risc0 icon indicating copy to clipboard operation
risc0 copied to clipboard

[Feature] Suppress `unused` warnings for GCC as well in Zirgen

Open weikengchen opened this issue 1 year ago • 3 comments

Feature

Not a lot of people try to compile from the source in Linux. At this moment, we can see the following warnings:

warning: cxx/step_verify_bytes.cpp: In function ‘risc0::Fp risc0::circuit::recursion::step_verify_bytes(void*, void (*)(void*, const char*, const char*, const risc0::Fp*, size_t, risc0::Fp*, size_t), size_t, size_t, risc0::Fp**)’:
warning: cxx/step_verify_bytes.cpp:33:10: warning: unused variable ‘mask’ [-Wunused-variable]
warning:    33 |   size_t mask = steps - 1;
warning:       |          ^~~~
warning: cxx/step_verify_bytes.cpp:32:28: warning: unused parameter ‘ctx’ [-Wunused-parameter]
warning:    32 | Fp step_verify_bytes(void* ctx, HostBridge host, size_t steps, size_t cycle, Fp** args) {
warning:       |                      ~~~~~~^~~
warning: cxx/step_verify_bytes.cpp:32:44: warning: unused parameter ‘host’ [-Wunused-parameter]
warning:    32 | Fp step_verify_bytes(void* ctx, HostBridge host, size_t steps, size_t cycle, Fp** args) {
warning:       |                                 ~~~~~~~~~~~^~~~
warning: cxx/step_verify_bytes.cpp:32:71: warning: unused parameter ‘cycle’ [-Wunused-parameter]
warning:    32 | Fp step_verify_bytes(void* ctx, HostBridge host, size_t steps, size_t cycle, Fp** args) {
warning:       |                                                                ~~~~~~~^~~~~
warning: cxx/step_verify_bytes.cpp:32:83: warning: unused parameter ‘args’ [-Wunused-parameter]
warning:    32 | Fp step_verify_bytes(void* ctx, HostBridge host, size_t steps, size_t cycle, Fp** args) {
warning:       |                                                                              ~~~~~^~~~

The same warnings would not appear for clang because the code explicitly tells clang to ignore those unused warnings.

#if defined(__clang__)
#pragma clang diagnostic ignored "-Wunused-parameter"
#pragma clang diagnostic ignored "-Wunused-variable"
#endif

However, the same is not done for GCC, which is likely going to be the more common compiler in Linux.

#if defined(__GNUG__)
#pragma GCC diagnostic ignored "-Wunused-parameter"
#pragma GCC diagnostic ignored "-Wunused-variable"
#endif

Motivation

For clearness's sake.

Implementation

See the above. But, I would admit that this issue is probably the lowest in priority and urgency.

weikengchen avatar Jan 18 '24 19:01 weikengchen

We've seen issues with gcc in terms of speed and sometimes crashes on certain versions. So we force clang in our builds. I think the issue here is that we should force the use of clang in the build.

flaub avatar Jan 18 '24 19:01 flaub

If gcc is problematic then it would be advisable to tell Linux builders to use clang... to build the binary, one has already installed a lot of dependency, and should be able to afford clang.

Note: this is about how to let the cc crate use clang.

weikengchen avatar Jan 18 '24 19:01 weikengchen

Seems to be just adding .compiler("clang") to cc builder. I have the hunch that this depends on how bad GCC is. If we see more occurrences that using GCC is faulty then this can be a must.

weikengchen avatar Jan 18 '24 19:01 weikengchen