Undefined behavior __builtin_clz(0) in th_encode_ctl().

The GCC sanitizing code activated in !31 (merged) trigger the following error when running 'make check'

  ../../lib/encode.c:1384:49: runtime error: passing zero to clz(),
    which is not a valid argument
    #0 0x7f0af5dae97d in th_encode_ctl ../../lib/encode.c:1384
    #1 0x7f0af5e2eb97 in theora_encode_init ../../lib/encapiwrapper.c:72
    #2 0x55fd4beb18b7 in granulepos_test_encode
     ../../tests/granulepos_theora.c:71
    #3 0x55fd4beb126d in main ../../tests/granulepos_theora.c:137
    #4 0x7f0af51d3249 in __libc_start_call_main
     ../sysdeps/nptl/libc_start_call_main.h:58
    #5 0x7f0af51d3304 in __libc_start_main_impl ../csu/libc-start.c:360
    #6 0x55fd4beb1300 in _start
     (/home/user/libtheora/build/tests/.libs/granulepos_theoraenc+0x2300)

This is lib/encode.c line 1384:

 _enc->state.info.keyframe_granule_shift=OC_CLAMPI(
  _enc->state.info.keyframe_granule_shift,
  OC_ILOG_32(keyframe_frequency_force-1),31);

Via the OC_CLZ32() macro this end up with several calls to __builtin_clz(keyframe_frequency_force-1), and when keyframe_frequency_force is 1, this end up with a undefined call to __builtin_clz(0), triggering the "passing zero to clz()" message.

The code should be adjusted to avoid this undefined behavior.

The result of the bsr instruction is undefined on x86, but that is very different from undefined C behavior: the CPU is not allowed to do absolutely anything if it encounters the former. I was not aware the compiler was treating this as the latter. If you look at the definition of OC_ILOG_32 in mathops.h, you will see that the value of the __builtin_clz() call is never used (the &-!!(_v) sets the value to zero when (_v) is zero, and leaves it unchanged otherwise). If the compiler is going to treat it this way, then I think the only valid solution is to use a conditional:

# define OC_ILOG_32((_v)?IC_ILOGNZ_32(_v):0))

You will need to do the same for OC_ILOG_64() below.

mentioned in commit 946c8974

mentioned in commit 5e57d38f

mentioned in merge request !33 (merged)

Draft fix is now available as !33 (merged).

mentioned in commit 0cb634ec

closed with commit 0cb634ec