iSAC entropy coder: Avoid signed integer overflow

iSAC entropy coder: Avoid signed integer overflow

By doing an unsigned instead of a signed addition, we get the exact
same machine code (in non-UBSan builds), but no longer trigger
undefined behavior since unsigned overflow is defined behavior.

BUG=webrtc:5485
Committed: https://crrev.com/806706875dac634a974f2ede1c4926a2e549fe47
Cr-Commit-Position: refs/heads/master@{#11776}

[kwiberg-webrtc, 2016-02-25 11:56:59]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kjellander@webrtc.org

[kwiberg-webrtc, 2016-02-25 11:57:00]

Except in one place (where I have a comment), this CL just changes a signed
32-bit addition to an unsigned 32-bit addition, resulting in the same machine
code but without the undefined behavior.

Note that adding 16777216 = 2^24 in an attempt to round to nearest while
dividing by 2^25 has no effect, since adding a constant to a uniformly random
uint32_t gives a uniformly random uint32_t. But the actual pseudo-random
sequence produced will of course be different, and I'm not feeling brave
today...

https://codereview.webrtc.org/1734883003/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/fix/source/entropy_coding.c
(right):

https://codereview.webrtc.org/1734883003/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/fix/source/entropy_coding.c:401:
dither2_Q7 = (int16_t)(((int32_t)(seed + 16777216)) >> 25);
Here, I'm actually changing the behavior. The old code did an unsigned right
shift, while the new does a signed right shift (like in all the other places).
The new behavior is obviously the correct one; the old code gave results in the
range [0,127] instead of [-64,63].

[kjellander_webrtc, 2016-02-25 12:02:10]

lgtm for tools/ubsan/blacklist.txt
passing the rest to Mr. Lundin

[hlundin-webrtc, 2016-02-25 14:40:03]

henrik.lundin@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[hlundin-webrtc, 2016-02-25 14:40:03]

henrik.lundin@webrtc.org changed required reviewers:
+ tina.legrand@webrtc.org

[hlundin-webrtc, 2016-02-25 14:40:04]

This lgtm, I suppose, but I have no idea what the changed range for the dither
value will actually bring about. Adding Tina who I think knows the code better.

[kwiberg-webrtc, 2016-02-25 14:49:15]

On 2016/02/25 14:40:04, hlundin-webrtc wrote:
> This lgtm, I suppose, but I have no idea what the changed range for the dither
> value will actually bring about. Adding Tina who I think knows the code
better.

I have no idea either. I just know that none of our bitexactness tests have
complained.

If it's deemed too risky, I can always restore that calculation to the old
erroneous form and just leave a comment.

[tlegrand-webrtc, 2016-02-26 10:21:41]

I think the dither change is safe to do, given that the result is still bit
exact. Dither is just added to randomize quantization noise, and seem to have no
real effect here. LGTM

https://codereview.webrtc.org/1734883003/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/isac/fix/source/entropy_coding.c
(right):

https://codereview.webrtc.org/1734883003/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/isac/fix/source/entropy_coding.c:401:
dither2_Q7 = (int16_t)(((int32_t)(seed + 16777216)) >> 25);
On 2016/02/25 11:57:00, kwiberg-webrtc wrote:
> Here, I'm actually changing the behavior. The old code did an unsigned right
> shift, while the new does a signed right shift (like in all the other places).
> The new behavior is obviously the correct one; the old code gave results in
the
> range [0,127] instead of [-64,63].

I think this changes should be fine.

[kwiberg-webrtc, 2016-02-26 10:50:31]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-02-26 10:50:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1734883003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1734883003/1

[commit-bot: I haz the power, 2016-02-26 10:52:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-26 10:52:21]

Description was changed from

==========
iSAC entropy coder: Avoid signed integer overflow

By doing an unsigned instead of a signed addition, we get the exact
same machine code (in non-UBSan builds), but no longer trigger
undefined behavior since unsigned overflow is defined behavior.

BUG=webrtc:5485
==========

to

==========
iSAC entropy coder: Avoid signed integer overflow

By doing an unsigned instead of a signed addition, we get the exact
same machine code (in non-UBSan builds), but no longer trigger
undefined behavior since unsigned overflow is defined behavior.

BUG=webrtc:5485

Committed: https://crrev.com/806706875dac634a974f2ede1c4926a2e549fe47
Cr-Commit-Position: refs/heads/master@{#11776}
==========

[commit-bot: I haz the power, 2016-02-26 10:52:22]

Patchset 1 (id:??) landed as
https://crrev.com/806706875dac634a974f2ede1c4926a2e549fe47
Cr-Commit-Position: refs/heads/master@{#11776}