Undefined behaviour shifting negative values in decode.c

Clang warn of a lot of undefined behavior in decode.c:

../../lib/decode.c:195:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0,10, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:198:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0,  0,1),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:208:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 1, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:210:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 2, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:212:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 3, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:214:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 4, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:216:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 5, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:220:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 1, -2,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:221:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 1, -3,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:227:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 6, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:228:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 7, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:229:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 8, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:230:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 9, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:236:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 2, -2,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:237:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 3, -2,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:238:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 2, -3,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:239:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 3, -3,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:242:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0,  0,1),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:253:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -1,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:257:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -2,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:260:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -3,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:262:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -4,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:264:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -5,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:266:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -6,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:270:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -7,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:271:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -8,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:277:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0, -9,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:278:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0,-10,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:279:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0,-11,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^
../../lib/decode.c:280:3: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
  OC_DCT_CW_PACK( 0, 0,-12,0),
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/decode.c:150:16: note: expanded from macro 'OC_DCT_CW_PACK'
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
 ~~~~~~~~~~~~~~^

I extractedthe relevant portions into a test program:

#include <inttypes.h>
#include <stdint.h>
#include <stdio.h>

#define OC_DCT_CW_RLEN_SHIFT (0)
#define OC_DCT_CW_EOB_SHIFT  (8)
#define OC_DCT_CW_FLIP_BIT   (20)
#define OC_DCT_CW_MAG_SHIFT  (21)

#if 1
#define OC_DCT_CW_PACK(_eobs,_rlen,_mag,_flip) \
 ((_eobs)<<OC_DCT_CW_EOB_SHIFT| \
 (_rlen)<<OC_DCT_CW_RLEN_SHIFT| \
 (_flip)<<OC_DCT_CW_FLIP_BIT| \
 (_mag)-(_flip)<<OC_DCT_CW_MAG_SHIFT)
#else
#define OC_DCT_CW_PACK(_eobs,_rlen,_mag,_flip) \
 ((_eobs)<<OC_DCT_CW_EOB_SHIFT| \
 (_rlen)<<OC_DCT_CW_RLEN_SHIFT| \
 (_flip)<<OC_DCT_CW_FLIP_BIT| \
 ((unsigned)((_mag)-(_flip)))<<OC_DCT_CW_MAG_SHIFT)
#endif

static const int32_t OC_DCT_CODE_WORD[]={
  OC_DCT_CW_PACK( 0,10, -1,0),
  OC_DCT_CW_PACK( 0, 1, -1,0),
  OC_DCT_CW_PACK( 0, 2, -1,0),
  OC_DCT_CW_PACK( 0, 3, -1,0),
  OC_DCT_CW_PACK( 0, 4, -1,0),
  OC_DCT_CW_PACK( 0, 5, -1,0),
  OC_DCT_CW_PACK( 0, 1, -2,0),
  OC_DCT_CW_PACK( 0, 1, -3,0),
  OC_DCT_CW_PACK( 0, 6, -1,0),
  OC_DCT_CW_PACK( 0, 7, -1,0),
  OC_DCT_CW_PACK( 0, 8, -1,0),
  OC_DCT_CW_PACK( 0, 9, -1,0),
  OC_DCT_CW_PACK( 0, 2, -2,0),
  OC_DCT_CW_PACK( 0, 3, -2,0),
  OC_DCT_CW_PACK( 0, 2, -3,0),
  OC_DCT_CW_PACK( 0, 3, -3,0),
  OC_DCT_CW_PACK( 0, 0, -1,0),
  OC_DCT_CW_PACK( 0, 0, -2,0),
  OC_DCT_CW_PACK( 0, 0, -3,0),
  OC_DCT_CW_PACK( 0, 0, -4,0),
  OC_DCT_CW_PACK( 0, 0, -5,0),
  OC_DCT_CW_PACK( 0, 0, -6,0),
  OC_DCT_CW_PACK( 0, 0, -7,0),
  OC_DCT_CW_PACK( 0, 0, -8,0),
  OC_DCT_CW_PACK( 0, 0, -9,0),
  OC_DCT_CW_PACK( 0, 0,-10,0),
  OC_DCT_CW_PACK( 0, 0,-11,0),
  OC_DCT_CW_PACK( 0, 0,-12,0),
};

int main(int argc, char *argv[])
{
  for (int i=0; i<sizeof(OC_DCT_CODE_WORD)/sizeof(OC_DCT_CODE_WORD[0]); i++) {
    printf("0x%" PRIx32 "\n", OC_DCT_CODE_WORD[i]);
  }
  return 0;
}

A workaround to get rid of the warning is to add the (unsigned) cast provided in the #if block above. Both with and without the casting, the output from the program look like this:

0xffe0000a
0xffe00001
0xffe00002
0xffe00003
0xffe00004
0xffe00005
0xffc00001
0xffa00001
0xffe00006
0xffe00007
0xffe00008
0xffe00009
0xffc00002
0xffc00003
0xffa00002
0xffa00003
0xffe00000
0xffc00000
0xffa00000
0xff800000
0xff600000
0xff400000
0xff200000
0xff000000
0xfee00000
0xfec00000
0xfea00000
0xfe800000

Given that shifting a negative number is undefined, I am not sure if these are the expected values in this table, and also unsure if hiding the warning using the cast is the correct approach. The output from this progr

C99 strikes again. Same as !23 (merged) I think the cleanest solution is to use a multiply in place of the shift, because that still works as expected in C99, the compiler will optimize it back to the shift, and if the result actually does overflow, you can still find out with a sanitizer (unlike with the unsigned cast).

mentioned in commit 1dbc6a09

mentioned in commit a768e356

closed with commit a768e356