martincameron / micromod

Music player libraries for MOD, S3M and XM formats.
BSD 3-Clause "New" or "Revised" License
161 stars 29 forks source link

Silence -Wstrict-overflow warnings #13

Closed orbea closed 7 years ago

orbea commented 7 years ago

Silences the following warnings in ibxm-ac/ibxm.c with gcc-7.1.0 and -O3.

gcc -O3 -ansi -pedantic -Wall -g xm2wav.c ibxm.c -o xm2wav
ibxm.c: In function ‘module_load’:
ibxm.c:95:4: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
  if( offset + length > data->length ) {
    ^
ibxm.c:95:4: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
  if( offset + length > data->length ) {
    ^

Please review to make sure my solution is correct, thanks!

orbea commented 7 years ago

@martincameron When you have a chance can you review this PR? Thanks!

martincameron commented 7 years ago

I suppose the warning means that the optimizer has determined that the function is never called with out-of-bounds parameters unless overflow occurs.

Changing the length parameter to "unsigned int" also avoids the warning, would prefer that.

Cheers, Martin

On 20 August 2017 at 14:42, orbea notifications@github.com wrote:

@martincameron https://github.com/martincameron When you have a chance can you review this PR? Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/martincameron/micromod/pull/13#issuecomment-323585796, or mute the thread https://github.com/notifications/unsubscribe-auth/ALFO0uQuzfJALfDZqXF52Kuys7CRQX0kks5saDfegaJpZM4O53TA .

orbea commented 7 years ago

Do you mean like this?

diff --git a/ibxm-ac/ibxm.c b/ibxm-ac/ibxm.c
index b7c1786..1a15dd2 100644
--- a/ibxm-ac/ibxm.c
+++ b/ibxm-ac/ibxm.c
@@ -86,7 +86,7 @@ static int log_2( int x ) {
    return y;
 }

-static char* data_ascii( struct data *data, int offset, int length, char *dest ) {
+static char* data_ascii( struct data *data, int offset, unsigned int length, char *dest ) {
    int idx, chr;
    memset( dest, 32, length );
    if( offset > data->length ) {

While that does silence the warnings it creates new ones as well...

ibxm.c: In function ‘data_ascii’:
ibxm.c:95:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if( offset + length > data->length ) {
                      ^
ibxm.c:98:20: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for( idx = 0; idx < length; idx++ ) {

This PR suffers from the same issue unfortunately...

martincameron commented 7 years ago

I've checked in a fix that seems to do the right thing.

Thanks a lot! Martin

On 22 August 2017 at 23:21, orbea notifications@github.com wrote:

Do you mean like this?

diff --git a/ibxm-ac/ibxm.c b/ibxm-ac/ibxm.c index b7c1786..1a15dd2 100644 --- a/ibxm-ac/ibxm.c +++ b/ibxm-ac/ibxm.c @@ -86,7 +86,7 @@ static int log_2( int x ) { return y; }

-static char data_ascii( struct data data, int offset, int length, char dest ) { +static char data_ascii( struct data data, int offset, unsigned int length, char dest ) { int idx, chr; memset( dest, 32, length ); if( offset > data->length ) {

While that does silence the warnings it creates new ones as well...

ibxm.c: In function ‘data_ascii’: ibxm.c:95:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if( offset + length > data->length ) { ^ ibxm.c:98:20: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for( idx = 0; idx < length; idx++ ) {

This PR suffers from the same issue unfortunately...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/martincameron/micromod/pull/13#issuecomment-324168218, or mute the thread https://github.com/notifications/unsubscribe-auth/ALFO0ifKtCdNH-5kzNBs5HMlQYM6XC19ks5sa1R6gaJpZM4O53TA .

orbea commented 7 years ago

Thanks, but I am still getting a warning here.

gcc -O3 -ansi -pedantic -Wall -g xm2wav.c ibxm.c -o xm2wav
ibxm.c: In function ‘module_load’:
ibxm.c:96:14: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
  if( end < 0 || end > data->length ) {
      ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
orbea commented 7 years ago

@martincameron, I updated my commit to attempt to correctly silence the last warning. With this I no longer get any warnings with gcc or clang using -O3, -Wall and -Wextra. Can you review this change to see if the solution is correct?

martincameron commented 7 years ago

Hmm. Well, that's because sizeof( int expression ) always returns 4!

This is a difficult warning to silence, I'm currently on GCC6, so I'm going to have to upgrade to GCC7 to reproduce what you're seeing.

Thanks, Martin

On 29 August 2017 at 02:36, orbea notifications@github.com wrote:

@martincameron https://github.com/martincameron, I updated my commit to attempt to correctly silence the last warning. With this I no longer get any warnings with gcc or clang using -O3, -Wall and -Wextra. Can you review this change to see if the solution is correct?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/martincameron/micromod/pull/13#issuecomment-325529149, or mute the thread https://github.com/notifications/unsubscribe-auth/ALFO0kCKHGNKJ-F8HO9qUKmFF-J1DkFKks5sc2ssgaJpZM4O53TA .

orbea commented 7 years ago

Alright, thanks for the feedback!

martincameron commented 7 years ago

Hi, I've finally got GCC7 up and running and checked-in a change that seems to silence the warning when compiling with "-O3 -Wall".

Cheers, Martin.

On 30 August 2017 at 22:20, orbea notifications@github.com wrote:

Alright, thanks for the feedback!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/martincameron/micromod/pull/13#issuecomment-326122679, or mute the thread https://github.com/notifications/unsubscribe-auth/ALFO0nKaOgyHe0ukQYLwJ1KvdCBR42hPks5sddIagaJpZM4O53TA .

orbea commented 7 years ago

Cheers, it works here too.