3 ms·

I prefer maximum warnings and use of #pragma to disable a warning at a specific point when I know the warning is useless/wrong. That way I can get to a totally

I prefer maximum warnings and use of #pragma to disable a warning at a specific point when I know the warning is useless/wrong. That way I can get to a totally clean build with zero warnings output. The last significant piece of C I wrote: # Turn on absolutely all warnings and turn them into errors CFLAGS += -g -Wall -Wextra -Wno-unused-parameter -Werror https://github.com/cloudflare/keyless/blob/master/Makefile#L44 https://github.com/cloudflare/keyless/blob/master/Makefile#L... The reason I do this is if I leave some warnings in I can get complacent "oh, there's always a few warnings" or "that's expected" and then miss something. So, I like all the warnings and warnings treated as errors.

Snetry 4y ago
well, clearly not "maximum warnings" since you are missing a lot of them. -pedantic -Wunreachable-code -Wcast-qual -Wswitch-default -Wconversion -Wshadow -Wstrict-aliasing -Winit-self -Wcast-align -Wpointer-arith -Wmissing-declarations -Wmissing-include-dirs -Wno-unused-parameter -Wuninitialized and thats not even all that are useful. Also, don't use -Werror if you can Warnings will change based on compiler, compiler version and a countless number of other factors that make working with code that turns them into errors a pain.
- throwway120385 4y ago
  I actually do what parent does for work projects but I also control the compiler and can decide when to upgrade. Like everything it's a trade-off and if I wanted wide support for different platforms I would consider your advice.
  - Snetry 4y ago
    Do you only control the compiler or the entire toolchain and system? If only the compiler you are still on very shaky grounds with -Werror. Even for the Linux kernel devs -Werror is too harsh.
- flohofwoe 4y ago
  A good strategy is to use -Werror in development mode and CI builds, but not in 'distribution mode' (or alternatively: use -Werror for your own code, but not for imported libraries).
loup-vaillant 4y ago
Man, you should try Clang's `-Weverything` it's… well you know what, I'll try that on my stupidly high-quality cryptographic code: $ make "CC=clang -std=c99" "CFLAGS=-O3 -Weverything" clang -std=c99 -O3 -Weverything -I src -I src/optional -fPIC -c -o lib/monocypher.o src/monocypher.c src/monocypher.c:262:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 tmp[64]; ^ src/monocypher.c:231:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 pool[16]; ^ src/monocypher.c:337:12: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] const u64 s0 = ctx->h[0] + (u64)s[0]; // s0 <= 1_fffffffe ^ In file included from src/monocypher.c:54: src/monocypher.h:289:9: warning: padding size of 'crypto_poly1305_ctx' with 4 bytes to alignment boundary [-Wpadded] typedef struct { ^ src/monocypher.c:410:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_blocks = message_size >> 4; ^ src/monocypher.c:437:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 c = 5; ^ src/monocypher.c:498:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 v0 = ctx->hash[0]; u64 v8 = iv[0]; ^ src/monocypher.c:621:10: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_words = message_size >> 3; ^ src/monocypher.c:600:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_blocks = message_size >> 7; ^ src/monocypher.c:640:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t hash_size = MIN(ctx->hash_size, 64); ^ src/monocypher.c:786:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 hash_area[1024]; ^ src/monocypher.c:874:10: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 next_slice = ((slice + 1) % 4) * segment_size; ^ src/monocypher.c:801:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int constant_time = config.algorithm != CRYPTO_ARGON2_D; ^ src/monocypher.c:1170:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] i32 q = (19 * t[9] + (((i32) 1) << 24)) >> 25; ^ src/monocypher.c:1337:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 isodd = s[0] & 1; ^ src/monocypher.c:1349:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int isdifferent = crypto_verify32(fs, gs); ^ src/monocypher.c:1433:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] i32 *quartic = t1; ^ src/monocypher.c:1500:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe x2, z2, x3, z3, t0, t1; ^ src/monocypher.c:1650:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 carry = 1; ^ src/monocypher.c:1676:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 B[8]; load32_le_buf(B, b, 8); ^ src/monocypher.c:1756:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(h->X, h->X); ^ src/monocypher.c:1956:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int lsb = v & (~v + 1); // smallest bit of v ^ src/monocypher.c:2015:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int h_digit = slide_step(&h_slide, P_W_WIDTH, i, h); ^ src/monocypher.c:1994:12: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] ge_cached lutA[P_W_SIZE]; ^ src/monocypher.c:2185:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe tmp_a, tmp_b; // temporaries for addition ^ src/monocypher.c:2540:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe t1, t2; ^ src/monocypher.c:2641:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(t1, t1); ^ src/monocypher.c:2696:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(t3, t3); // t3 = sqrt(-1 / non_square * u * (u+A)) ^ src/monocypher.c:2775:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 t[16] = {0}; ^ src/monocypher.c:2815:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 m_scl[8]; ^ src/monocypher.c:2870:22: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] crypto_poly1305_ctx poly_ctx; // auto wiped... ^ src/monocypher.c:2925:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int mismatch = crypto_verify16(mac, real_mac); ^ src/monocypher.c:2953:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int mismatch = crypto_aead_read(&ctx, plain_text, mac, ad, ad_size, ^ 33 warnings generated. clang -std=c99 -O3 -Weverything -I src -I src/optional -fPIC -c -o lib/monocypher-ed25519.o src/optional/monocypher-ed25519.c src/optional/monocypher-ed25519.c:165:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 in = K[i16 + j] + ctx->input[j]; ^ src/optional/monocypher-ed25519.c:158:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t i16 = 0; ^ 2 warnings generated. ar cr lib/libmonocypher.a lib/monocypher.o lib/monocypher-ed25519.o clang -std=c99 -O3 -Weverything -shared -Wl,-soname,libmonocypher.so.4 -o lib/libmonocypher.so.4 lib/monocypher.o lib/monocypher-ed25519.o ln -sf `basename lib/libmonocypher.so.4` lib/libmonocypher.so doc/doc_gen.sh --- Well… Yeah. That is cranking up warnings up to 11. Now this is C99 we're talking about, how about removing that obviously useless `-Wdeclaration-after-statement`? $ make "CC=clang -std=c99" "CFLAGS=-O3 -Weverything -Wno-declaration-after-statement" clang -std=c99 -O3 -Weverything -Wno-declaration-after-statement -I src -I src/optional -fPIC -c -o lib/monocypher.o src/monocypher.c In file included from src/monocypher.c:54: src/monocypher.h:289:9: warning: padding size of 'crypto_poly1305_ctx' with 4 bytes to alignment boundary [-Wpadded] typedef struct { ^ 1 warning generated. Ah, this one's may be real. Let's see this struct: typedef struct { // Do not rely on the size or contents of this type, // for they may change without notice. uint8_t c[16]; // chunk of the message size_t c_idx; // How many bytes are there in the chunk. uint32_t r [4]; // constant multiplier (from the secret key) uint32_t pad[4]; // random number added at the end (from the secret key) uint32_t h [5]; // accumulated hash } crypto_poly1305_ctx; Ah, I see: we end by an odd number of 32-bit words, and in the 64-bit architecture I'm compiling this on, there will be 4 bytes of padding. But then I have two problems: first, who cares? Second, I also target 32-bit architectures on which there will not be any padding, and adding it manually would be wasteful. There's the `#pragma` to silence the warning of course, but it's ugly, and I'm not sure it is strictly portable (unless the standard says compilers should ignore the `#pragma` if they don't understand it, but I confess haven't checked).

Snetry 4y ago

well, clearly not "maximum warnings" since you are missing a lot of them. -pedantic -Wunreachable-code -Wcast-qual -Wswitch-default -Wconversion -Wshadow -Wstrict-aliasing -Winit-self -Wcast-align -Wpointer-arith -Wmissing-declarations -Wmissing-include-dirs -Wno-unused-parameter -Wuninitialized and thats not even all that are useful. Also, don't use -Werror if you can Warnings will change based on compiler, compiler version and a countless number of other factors that make working with code that turns them into errors a pain.

throwway120385 4y ago
I actually do what parent does for work projects but I also control the compiler and can decide when to upgrade. Like everything it's a trade-off and if I wanted wide support for different platforms I would consider your advice.
- Snetry 4y ago
  Do you only control the compiler or the entire toolchain and system? If only the compiler you are still on very shaky grounds with -Werror. Even for the Linux kernel devs -Werror is too harsh.
flohofwoe 4y ago
A good strategy is to use -Werror in development mode and CI builds, but not in 'distribution mode' (or alternatively: use -Werror for your own code, but not for imported libraries).

loup-vaillant 4y ago

Man, you should try Clang's `-Weverything` it's… well you know what, I'll try that on my stupidly high-quality cryptographic code: $ make "CC=clang -std=c99" "CFLAGS=-O3 -Weverything" clang -std=c99 -O3 -Weverything -I src -I src/optional -fPIC -c -o lib/monocypher.o src/monocypher.c src/monocypher.c:262:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 tmp[64]; ^ src/monocypher.c:231:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 pool[16]; ^ src/monocypher.c:337:12: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] const u64 s0 = ctx->h[0] + (u64)s[0]; // s0 <= 1_fffffffe ^ In file included from src/monocypher.c:54: src/monocypher.h:289:9: warning: padding size of 'crypto_poly1305_ctx' with 4 bytes to alignment boundary [-Wpadded] typedef struct { ^ src/monocypher.c:410:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_blocks = message_size >> 4; ^ src/monocypher.c:437:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 c = 5; ^ src/monocypher.c:498:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 v0 = ctx->hash[0]; u64 v8 = iv[0]; ^ src/monocypher.c:621:10: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_words = message_size >> 3; ^ src/monocypher.c:600:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t nb_blocks = message_size >> 7; ^ src/monocypher.c:640:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t hash_size = MIN(ctx->hash_size, 64); ^ src/monocypher.c:786:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 hash_area[1024]; ^ src/monocypher.c:874:10: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 next_slice = ((slice + 1) % 4) * segment_size; ^ src/monocypher.c:801:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int constant_time = config.algorithm != CRYPTO_ARGON2_D; ^ src/monocypher.c:1170:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] i32 q = (19 * t[9] + (((i32) 1) << 24)) >> 25; ^ src/monocypher.c:1337:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u8 isodd = s[0] & 1; ^ src/monocypher.c:1349:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int isdifferent = crypto_verify32(fs, gs); ^ src/monocypher.c:1433:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] i32 *quartic = t1; ^ src/monocypher.c:1500:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe x2, z2, x3, z3, t0, t1; ^ src/monocypher.c:1650:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 carry = 1; ^ src/monocypher.c:1676:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 B[8]; load32_le_buf(B, b, 8); ^ src/monocypher.c:1756:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(h->X, h->X); ^ src/monocypher.c:1956:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int lsb = v & (~v + 1); // smallest bit of v ^ src/monocypher.c:2015:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int h_digit = slide_step(&h_slide, P_W_WIDTH, i, h); ^ src/monocypher.c:1994:12: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] ge_cached lutA[P_W_SIZE]; ^ src/monocypher.c:2185:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe tmp_a, tmp_b; // temporaries for addition ^ src/monocypher.c:2540:5: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] fe t1, t2; ^ src/monocypher.c:2641:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(t1, t1); ^ src/monocypher.c:2696:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int is_square = invsqrt(t3, t3); // t3 = sqrt(-1 / non_square * u * (u+A)) ^ src/monocypher.c:2775:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 t[16] = {0}; ^ src/monocypher.c:2815:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u32 m_scl[8]; ^ src/monocypher.c:2870:22: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] crypto_poly1305_ctx poly_ctx; // auto wiped... ^ src/monocypher.c:2925:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int mismatch = crypto_verify16(mac, real_mac); ^ src/monocypher.c:2953:6: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] int mismatch = crypto_aead_read(&ctx, plain_text, mac, ad, ad_size, ^ 33 warnings generated. clang -std=c99 -O3 -Weverything -I src -I src/optional -fPIC -c -o lib/monocypher-ed25519.o src/optional/monocypher-ed25519.c src/optional/monocypher-ed25519.c:165:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] u64 in = K[i16 + j] + ctx->input[j]; ^ src/optional/monocypher-ed25519.c:158:9: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement] size_t i16 = 0; ^ 2 warnings generated. ar cr lib/libmonocypher.a lib/monocypher.o lib/monocypher-ed25519.o clang -std=c99 -O3 -Weverything -shared -Wl,-soname,libmonocypher.so.4 -o lib/libmonocypher.so.4 lib/monocypher.o lib/monocypher-ed25519.o ln -sf `basename lib/libmonocypher.so.4` lib/libmonocypher.so doc/doc_gen.sh --- Well… Yeah. That is cranking up warnings up to 11. Now this is C99 we're talking about, how about removing that obviously useless `-Wdeclaration-after-statement`? $ make "CC=clang -std=c99" "CFLAGS=-O3 -Weverything -Wno-declaration-after-statement" clang -std=c99 -O3 -Weverything -Wno-declaration-after-statement -I src -I src/optional -fPIC -c -o lib/monocypher.o src/monocypher.c In file included from src/monocypher.c:54: src/monocypher.h:289:9: warning: padding size of 'crypto_poly1305_ctx' with 4 bytes to alignment boundary [-Wpadded] typedef struct { ^ 1 warning generated. Ah, this one's may be real. Let's see this struct: typedef struct { // Do not rely on the size or contents of this type, // for they may change without notice. uint8_t c[16]; // chunk of the message size_t c_idx; // How many bytes are there in the chunk. uint32_t r [4]; // constant multiplier (from the secret key) uint32_t pad[4]; // random number added at the end (from the secret key) uint32_t h [5]; // accumulated hash } crypto_poly1305_ctx; Ah, I see: we end by an odd number of 32-bit words, and in the 64-bit architecture I'm compiling this on, there will be 4 bytes of padding. But then I have two problems: first, who cares? Second, I also target 32-bit architectures on which there will not be any padding, and adding it manually would be wasteful. There's the `#pragma` to silence the warning of course, but it's ugly, and I'm not sure it is strictly portable (unless the standard says compilers should ignore the `#pragma` if they don't understand it, but I confess haven't checked).