4 ms·

I wonder how many of them would be caught using a very strict compiler flag regime. For the winner, perhaps the gcc flag, -Wmissing-prototypes would catch it?

by asgfoi 11y ago

nkurz 11y ago
No, that doesn't catch it because "float_t" is properly defined as a 'float' by including <math.h>, instead of a 'double' in the local header. But I thought you might be on to something with adding extra warnings, so I tried it out on http://gcc.godbolt.org http://gcc.godbolt.org. Nope, no warnings at all for spectral_contrast.c with GCC, ICC, or Clang even with "-Wall -Wextra -pedantic". Then I thought to try it on MSVC at http://webcompiler.cloudapp.net/ http://webcompiler.cloudapp.net/. To my surprise, it caught it pretty clearly with /W4: main.cpp(11): warning C4244: '/=': conversion from 'double' to 'float_t', possible loss of data With that hint, I searched for other GCC warnings and found -Wconversion. Indeed, with that (or -Wfloat-conversion) GCC picks up the scent pretty well: http://goo.gl/9xq3fG http://goo.gl/9xq3fG In function 'void normalize(float_t*, int)': 11 : warning: conversion to 'float_t {aka float}' from 'double' may alter its value [-Wfloat-conversion] ICC gives an excellent error message with -Wconversion also, perhaps the clearest of the bunch: http://goo.gl/cjXLjq http://goo.gl/cjXLjq warning #2259: non-pointer conversion from "double" to "float_t={float}" may lose significant bits for(i = 0; i < length; i++) v[i] /= magnitude; Clang remained silent with all the options I tried, but perhaps I missed the right one.
- saghul 11y ago
  Did you try with -Weverything on Clang? That will enable more warnings than -Wall and -Wextra.
  - 3JPLW 11y ago
    $ clang -Wall -Weverything -pedantic -c -o spectral_contrast.o spectral_contrast.c spectral_contrast.c:16:8: warning: no previous prototype for function 'spectral_contrast' [-Wmissing-prototypes] double spectral_contrast(float_t *a, float_t *b, int length) { ^ 1 warning generated.
  - nkurz 11y ago
    Thanks for mentioning that option. I hadn't known about it. Clang does give a warning with -Weverything, but I don't understand what it means: http://goo.gl/M9qjmx http://goo.gl/M9qjmx 13 : warning: no previous prototype for function spectral_contrast' [-Wmissing-prototypes] double spectral_contrast(float_t *a, float_t *b, int length) { It gives the same warning if I change all the float_t's to float, or if I change all the float_t's to double, so I think it's not actually useful or relevant.
    - 3JPLW 11y ago
      It's effectively catching the fact that `spectral_contrast.c` does not `#include "match.h"` — this is indeed directly a part of the underhandedness. The warning occurs because the function is not static — and therefore callable by other modules. Since it's missing a forward definition (a previous prototype), those modules must blindly declare the prototypes themselves… and their prototypes can get out of sync with the actual definition. That's what's happening here. `match.h` has the forward prototype, but it's very subtly different from the definition. Were match.h included, there'd be a much more glaring warning (or maybe even error).
- asgfoi 11y ago
  I tried to compile it, and I also couldn't find any other flag, than -Wconversion, that would detect this: for(i = 0; i < length; i++) sum += a[i] * b[i]; ^ I managed to hide the error if return values, which are double, are replaced with float_t. I believe in that case the bug stays in, but -Wconversion doesn't detect it. Of course the warning can always be silenced by doing something like: for(i = 0; i < length; i++) sum += ( double )( a[i] * b[i] ); and adding a misleading comment about the explicit cast.
  - scatters 11y ago
    The interesting thing about changing return types is that you can alter the return type of spectral_contrast to float_t without affecting behavior, since float is promoted to double for argument passing (it's passed and returned in the xmm registers).

I wonder how many of them would be caught using a very strict compiler flag regime. For the winner, perhaps the gcc flag, -Wmissing-prototypes would catch it?

by asgfoi 11y ago

I wonder how many of them would be caught using a very strict compiler flag regime. For the winner, perhaps the gcc flag, -Wmissing-prototypes would catch it?

nkurz 11y ago

No, that doesn't catch it because "float_t" is properly defined as a 'float' by including <math.h>, instead of a 'double' in the local header. But I thought you might be on to something with adding extra warnings, so I tried it out on http://gcc.godbolt.org http://gcc.godbolt.org. Nope, no warnings at all for spectral_contrast.c with GCC, ICC, or Clang even with "-Wall -Wextra -pedantic". Then I thought to try it on MSVC at http://webcompiler.cloudapp.net/ http://webcompiler.cloudapp.net/. To my surprise, it caught it pretty clearly with /W4: main.cpp(11): warning C4244: '/=': conversion from 'double' to 'float_t', possible loss of data With that hint, I searched for other GCC warnings and found -Wconversion. Indeed, with that (or -Wfloat-conversion) GCC picks up the scent pretty well: http://goo.gl/9xq3fG http://goo.gl/9xq3fG In function 'void normalize(float_t*, int)': 11 : warning: conversion to 'float_t {aka float}' from 'double' may alter its value [-Wfloat-conversion] ICC gives an excellent error message with -Wconversion also, perhaps the clearest of the bunch: http://goo.gl/cjXLjq http://goo.gl/cjXLjq warning #2259: non-pointer conversion from "double" to "float_t={float}" may lose significant bits for(i = 0; i < length; i++) v[i] /= magnitude; Clang remained silent with all the options I tried, but perhaps I missed the right one.

saghul 11y ago
Did you try with -Weverything on Clang? That will enable more warnings than -Wall and -Wextra.
- 3JPLW 11y ago
  $ clang -Wall -Weverything -pedantic -c -o spectral_contrast.o spectral_contrast.c spectral_contrast.c:16:8: warning: no previous prototype for function 'spectral_contrast' [-Wmissing-prototypes] double spectral_contrast(float_t *a, float_t *b, int length) { ^ 1 warning generated.
- nkurz 11y ago
  Thanks for mentioning that option. I hadn't known about it. Clang does give a warning with -Weverything, but I don't understand what it means: http://goo.gl/M9qjmx http://goo.gl/M9qjmx 13 : warning: no previous prototype for function spectral_contrast' [-Wmissing-prototypes] double spectral_contrast(float_t *a, float_t *b, int length) { It gives the same warning if I change all the float_t's to float, or if I change all the float_t's to double, so I think it's not actually useful or relevant.
  - 3JPLW 11y ago
    It's effectively catching the fact that `spectral_contrast.c` does not `#include "match.h"` — this is indeed directly a part of the underhandedness. The warning occurs because the function is not static — and therefore callable by other modules. Since it's missing a forward definition (a previous prototype), those modules must blindly declare the prototypes themselves… and their prototypes can get out of sync with the actual definition. That's what's happening here. `match.h` has the forward prototype, but it's very subtly different from the definition. Were match.h included, there'd be a much more glaring warning (or maybe even error).
asgfoi 11y ago
I tried to compile it, and I also couldn't find any other flag, than -Wconversion, that would detect this: for(i = 0; i < length; i++) sum += a[i] * b[i]; ^ I managed to hide the error if return values, which are double, are replaced with float_t. I believe in that case the bug stays in, but -Wconversion doesn't detect it. Of course the warning can always be silenced by doing something like: for(i = 0; i < length; i++) sum += ( double )( a[i] * b[i] ); and adding a misleading comment about the explicit cast.
- scatters 11y ago
  The interesting thing about changing return types is that you can alter the return type of spectral_contrast to float_t without affecting behavior, since float is promoted to double for argument passing (it's passed and returned in the xmm registers).