RFC: Enable and fix paranoid warnings #33

theuni commented at 5:11 PM on June 20, 2014: contributor

This is not meant for merge as-is, but rather to discuss whether it's worth doing, and if so, how it might be done correctly.

Abstract: If a baseline can be established where compilers (with all paranoid checks enabled) produce zero warnings, bugs in new code become much easier to spot.

This PR adds 2 configure switches: enable all paranoid warnings (and filter out the few that we don't care about), and turn warnings into errors. When combined, the pull-tester will fail when a new warning is introduced. It also adds clang to the build matrix in order to add a new batch of warnings (-Weverything)

The results are here: https://travis-ci.org/theuni/secp256k1/builds/28015905

Obviously most of these warnings are completely harmless and have no consequences, but a few legitimate bugs turn up as well. In a heavy math/crypto lib, type narrowing and implicit sign changes can be especially dangerous, so I would argue that these warnings are actually quite valuable.

Also, obviously the warnings that compilers enable with "-Wall -Wextra -Weverything" are entirely arbitrary, so the list of filtered warnings will likely evolve a bit.

Assuming this work is wanted, I see 2 possible ways to go about it:

as done in this pr. Quell all warnings so that the pull tester is happy, then begin fixing them up correctly. Please read the commit message from https://github.com/theuni/secp256k1/commit/30fba32ddab28d034afdd530d8dbd8d524832619 before pointing out the stupid changes in that commit.
don't flip the 'all warnings are errors' switch, and fix up each case properly. then flip the switch.

build: add autogen. How was this missing? d40908763a

build: add travis support

This adds a huge matrix of builds. The undesirable ones can be removed.

b0cc21f896

gmaxwell commented at 5:23 PM on June 20, 2014: contributor

For things like where you've added the casts to change sign conversions without (as you believe) changing behavior, what I'd expect to find is that the objdump -d on stripped outputs doesn't change. Did you try something like that?

-Werror is something that can't generally be shipped in production because compiler warnings are not deterministic from version to version.

theuni commented at 5:26 PM on June 20, 2014: contributor

@gmaxwell That's a great idea. I'm sure I bungled a few of em. I'll do that.

As for -Werror, I completely agree. It's off by default, and really only meant to be used by the pull-tester (where we're in control of the compilers used).

gmaxwell commented at 5:28 PM on June 20, 2014: contributor

Cool, make sure to try the different compile options (endomorphisms / openssl / gmp / 32 bit).

jgarzik commented at 5:33 PM on June 20, 2014: none

General comment/principle: casts are dangerous. They hide bugs, as we saw many times in the kernel. Programmers cast to shut up a warning, rather than (1) take a hard look at the asm output, and (2) re-examine the types used, to see if a type-related change can be made instead.

A cast may be expedient, it may be necessary, but it effectively kills any type-related warnings after the cast.

theuni commented at 5:40 PM on June 20, 2014: contributor

@jgarzik Agreed, but at least the casts must be explicit. In the ones shown in https://github.com/theuni/secp256k1/commit/30fba32ddab28d034afdd530d8dbd8d524832619, all of those casts were already happening, they just might not have been obvious.

Also, the casts as done in that commit are completely wrong. I agree. Most would be fixed properly by simply using the correct types in the first place. This one shows up often, for example:

function foo(unsigned int a)
{
    int b;
    for (b=0; b != a; b++)
        ...
}

Obviously the right thing to do there is just use an unsigned int for b. But I attempted to avoid mixing functional changes with cosmetic ones here.

gmaxwell commented at 5:44 PM on June 20, 2014: contributor

Using unsigned in loop counters frequently has performance implications because it weakens the compiler's range analysis. It also often causes bugs inside the loop body when the fact that unsigned is the stronger type causes surprising interior promotions in statements referencing the loop counter.

warnings: add compiler warning options to configure

These are mainly useful for the pull-tester. Once stable, it may be reasonable
to turn the warnings on by default.

6cbb6c6ca1

warnings: enable quiet builds f180ef783a

warnings: use new warning settings and add clang builder 27165bea45

warnings: remove unused variables fc022d020a

warnings: initialize arrays to 0

foo[] = {} is a gnu extension.

6180bf32f2

warnings: shut up about unused params 7fd39e2e2c

warnings: let the external functions see their prototypes

They may need to set attributes like visibility=default.

2738576178

warnings: make warnings errors and let's see what breaks e45d26e53f

warnings: fix possible overlength string dc8845840e

warnings: unsigned can't be negative ac8bc0014b

warnings: make gcc happy with parentheses 0469701602

warnings: fix struct padding. This is the _wrong_ fix.

There are several places that use sizeof(struct) for tests, where the struct's
size may be padded by the compiler. Those need to be sanitized.

11718c9a58

warnings: fix pedantic compiler warnings

Most of these are related to implicit sign conversion and width narrowing. They
have been "fixed" with casts in order to create a starting point for fixing
them correctly. Many of the changes in this commit are most definitely logically
_wrong_, but attempt to be consistent without breaking any functionality.

For example, the following old code:

  unsigned int a;
  int b = a;
  uint64_t c;
  uint32_t d = c;

Has become:

  unsigned int foo;
  int bar = (int)foo;
  uint64_t c;
  uint32_t d = (uint32_t)c;

This of course does nothing to fix any consequences of the narrowing and sign
conversions, but the casts become explicit.

By doing this as a first step, any attempt to fix the explicit casts more
correctly, while introducing a new warning, will be noticed by the pull-tester.

22c0211945

warnings: explicitly call out __int128 as a gnu extension

Otherwise -pedantic (correctly) complains.

cd3f6850d6

in src/group.h:None in cd3f6850d6

   8 | @@ -9,6 +9,7 @@
   9 |  #include "field.h"
  10 |  
  11 |  /** A group element of the secp256k1 curve, in affine coordinates. */
  12 | +#pragma pack(1)

sipa commented at 11:58 PM on June 26, 2014:

Why?

theuni commented at 12:00 AM on June 27, 2014:

sizeof(secp256k1_ge_consts_t)

sipa commented at 12:01 AM on June 27, 2014:

??

in src/num_gmp_impl.h:None in cd3f6850d6

  85 | @@ -85,6 +86,7 @@ void static secp256k1_num_add_abs(secp256k1_num_t *r, const secp256k1_num_t *a,
  86 |  
  87 |  void static secp256k1_num_sub_abs(secp256k1_num_t *r, const secp256k1_num_t *a, const secp256k1_num_t *b) {
  88 |      mp_limb_t c = mpn_sub(r->data, a->data, a->limbs, b->data, b->limbs);
  89 | +    (void)c;

sipa commented at 11:59 PM on June 26, 2014:

What is this for?

theuni commented at 12:01 AM on June 27, 2014:

c is unused. Obviously the right thing to do is just not create it, but I was trying to leave the result unaffected.

theuni closed this on Dec 12, 2014