kode54 / dumb

[Semi unmaintained] Dynamic Universal Music Bibliotheque - DUMB - Module/tracker based music format parser and player library -- Please consider using libopenmpt instead of this, it's considerably faster and produces similar or better rendering quality
Other
128 stars 26 forks source link

Warning: volr uninitialized #56

Closed SimonN closed 7 years ago

SimonN commented 7 years ago

Edit: PR #58 has a patch for this.


When I build DUMB master 6e924e6970 with gcc version 7.2.0 in release mode, the build is error-free, but still shows the warning below.

Unfortunately, dumb_resample is long and nasty to read, therefore I haven't investigated further.

[ 19%] Building C object CMakeFiles/dumb.dir/src/helpers/resample.c.o
In file included from /home/simon/c/notown/dumb/src/helpers/resample.c:162:0:
/home/simon/c/notown/dumb/src/helpers/resamp3.inc: In function ‘dumb_resample_1_1’:
/home/simon/c/notown/dumb/src/helpers/resample.inc:134:72: warning: ‘volr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define RETURN_MONO_DEST_VOLUME_VARIABLES if ( volume ) volume->volume = volr
                                                                        ^
/home/simon/c/notown/dumb/src/helpers/resample.inc:117:41: note: ‘volr’ was declared here
 #define MONO_DEST_VOLUME_VARIABLES vol, volr, vold, volt, volm
                                         ^
/home/simon/c/notown/dumb/src/helpers/resamp2.inc:93:26: note: in expansion of macro ‘MONO_DEST_VOLUME_VARIABLES’
 #define VOLUME_VARIABLES MONO_DEST_VOLUME_VARIABLES
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/simon/c/notown/dumb/src/helpers/resamp3.inc:50:8: note: in expansion of macro ‘VOLUME_VARIABLES’
  float VOLUME_VARIABLES;
        ^~~~~~~~~~~~~~~~
In file included from /home/simon/c/notown/dumb/src/helpers/resample.c:183:0:
/home/simon/c/notown/dumb/src/helpers/resamp3.inc: In function ‘dumb_resample_16_1_1’:
/home/simon/c/notown/dumb/src/helpers/resample.inc:134:72: warning: ‘volr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define RETURN_MONO_DEST_VOLUME_VARIABLES if ( volume ) volume->volume = volr
                                                                        ^
/home/simon/c/notown/dumb/src/helpers/resample.inc:117:41: note: ‘volr’ was declared here
 #define MONO_DEST_VOLUME_VARIABLES vol, volr, vold, volt, volm
                                         ^
/home/simon/c/notown/dumb/src/helpers/resamp2.inc:93:26: note: in expansion of macro ‘MONO_DEST_VOLUME_VARIABLES’
 #define VOLUME_VARIABLES MONO_DEST_VOLUME_VARIABLES
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/simon/c/notown/dumb/src/helpers/resamp3.inc:50:8: note: in expansion of macro ‘VOLUME_VARIABLES’
  float VOLUME_VARIABLES;
        ^~~~~~~~~~~~~~~~
In file included from /home/simon/c/notown/dumb/src/helpers/resample.c:190:0:
/home/simon/c/notown/dumb/src/helpers/resamp3.inc: In function ‘dumb_resample_8_1_1’:
/home/simon/c/notown/dumb/src/helpers/resample.inc:134:72: warning: ‘volr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define RETURN_MONO_DEST_VOLUME_VARIABLES if ( volume ) volume->volume = volr
                                                                        ^
/home/simon/c/notown/dumb/src/helpers/resample.inc:117:41: note: ‘volr’ was declared here
 #define MONO_DEST_VOLUME_VARIABLES vol, volr, vold, volt, volm
                                         ^
/home/simon/c/notown/dumb/src/helpers/resamp2.inc:93:26: note: in expansion of macro ‘MONO_DEST_VOLUME_VARIABLES’
 #define VOLUME_VARIABLES MONO_DEST_VOLUME_VARIABLES
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/simon/c/notown/dumb/src/helpers/resamp3.inc:50:8: note: in expansion of macro ‘VOLUME_VARIABLES’
  float VOLUME_VARIABLES;
        ^~~~~~~~~~~~~~~~
SimonN commented 7 years ago

The long dumb_resample looks like this:

long dumb_resample(...)
{
    int dt, inv_dt;
    float VOLUME_VARIABLES; // declares volr
    // ... lots of code ...
    RETURN_VOLUME_VARIABLES; // expands to if ( volume ) volume->volume = volr
    return done;
}

And the error is that we assign from this volr even though it was never initialized.

Simplifying this function in my mind was 80 % of the detective work. The variable should probably be set in SET_VOLUME_VARIABLES. Pull request incoming. :-)

kode54 commented 7 years ago

And volr is never assigned in that case, if you'll note that volume has been assigned a NULL pointer.

SimonN commented 7 years ago

Would you prefer a different solution? Or were the warnings in the build acceptable for you?

dumb_render is very long, and doesn't clarify that volr is OK to be uninitialized. It looked much more like forgotten assignment than deliberate omission for speed.

Another way to make that intent clearear is to declare volume as const DUMB_VOLUME_RAMP_INFO *const volume, comment the end of the function, and then disable the warning with a GCC pragma.

kode54 commented 7 years ago

No, fixing the warnings is fine. I just didn't notice them before, and wrote the code to assume that variable would be unused, and therefore not need to be initialized. The warning checker has no way of knowing easily that the "volume" variable will not be reset back to non-NULL before the end of the function. It's quite a complex mess that I still haven't fully recovered from the old macro mess.