Fix an UBSan error (signed overflow) in saturating addition and subtraction

Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are supposed to handle overflow gracefully, and they probably did. But since the overflow handling depended on undefined behavior, a sufficiently smart optimizing compiler would have realized that it could just ignore the possibility of overflow and omit all the overflow handling code, leaving only the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code (tested with clang 3.9 with -O2).

BUG=chromium:601728
Committed: https://crrev.com/bca568bfc59f944335510762f47fe4414ea1273e
Cr-Commit-Position: refs/heads/master@{#12846}

[kwiberg-webrtc, 2016-05-20 08:54:07]

Description was changed from

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

The new implementations, unlike the old ones, result in branch-free
code (tested with clang 3.9 with -O2).

BUG=chromium:601728
==========

to

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are
supposed to handle overflow gracefully, and they probably did. But since the
overflow handling depended on undefined behavior, a sufficiently smart
optimizing compiler would have realized that it could just ignore the
possibility of overflow and omit all the overflow handling code, leaving only
the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code
(tested with clang 3.9 with -O2).

BUG=chromium:601728
==========

[kwiberg-webrtc, 2016-05-20 08:57:15]

kwiberg@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[kwiberg-webrtc, 2016-05-20 08:57:16]

I think I've finally found a CL where you may not object to my use of
single-letter variable names. Really!

[tlegrand-webrtc, 2016-05-20 11:07:14]

Enjoy, it's not gonna happen again. ;)

A few nits, then LGTM.

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
File webrtc/common_audio/signal_processing/include/spl_inl.h (right):

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
webrtc/common_audio/signal_processing/include/spl_inl.h:39: // Do the addition
in unsigned numbers, since signed overflow is UB.
UB? Please write undefined behavior instead.

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
webrtc/common_audio/signal_processing/include/spl_inl.h:46: return sum < 0 ?
INT32_MAX : INT32_MIN;
Clever!

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
webrtc/common_audio/signal_processing/include/spl_inl.h:52: // Do the
subtraction in unsigned numbers, since signed overflow is UB.
Here as well.

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
File webrtc/common_audio/signal_processing/signal_processing_unittest.cc
(right):

https://codereview.chromium.org/2002523002/diff/20001/webrtc/common_audio/sig...
webrtc/common_audio/signal_processing/signal_processing_unittest.cc:126:
TEST_F(SplTest, AddSatW32) {
AddSubSatW32? Or some other naming, telling that both Add and Sub are tested.

[kwiberg-webrtc, 2016-05-23 08:29:47]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-05-23 08:30:04]

New patch set posted.

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/include/spl_inl.h (right):

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/include/spl_inl.h:39: // Do the addition
in unsigned numbers, since signed overflow is UB.
On 2016/05/20 11:07:14, tlegrand-webrtc wrote:
> UB? Please write undefined behavior instead.

Done.

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/include/spl_inl.h:46: return sum < 0 ?
INT32_MAX : INT32_MIN;
On 2016/05/20 11:07:14, tlegrand-webrtc wrote:
> Clever!

Acknowledged.

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/include/spl_inl.h:52: // Do the
subtraction in unsigned numbers, since signed overflow is UB.
On 2016/05/20 11:07:14, tlegrand-webrtc wrote:
> Here as well.

Done.

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/signal_processing_unittest.cc
(right):

https://codereview.webrtc.org/2002523002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/signal_processing_unittest.cc:126:
TEST_F(SplTest, AddSatW32) {
On 2016/05/20 11:07:14, tlegrand-webrtc wrote:
> AddSubSatW32? Or some other naming, telling that both Add and Sub are tested.

Done.

[kwiberg-webrtc, 2016-05-23 08:30:14]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-05-23 08:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tina.legrand@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2002523002/#ps40001 (title:
"review suggestions")

[commit-bot: I haz the power, 2016-05-23 08:30:22]

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

[commit-bot: I haz the power, 2016-05-23 10:30:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 10:30:50]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-05-23 10:49:22]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-05-23 10:49:26]

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

[commit-bot: I haz the power, 2016-05-23 11:07:05]

Description was changed from

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are
supposed to handle overflow gracefully, and they probably did. But since the
overflow handling depended on undefined behavior, a sufficiently smart
optimizing compiler would have realized that it could just ignore the
possibility of overflow and omit all the overflow handling code, leaving only
the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code
(tested with clang 3.9 with -O2).

BUG=chromium:601728
==========

to

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are
supposed to handle overflow gracefully, and they probably did. But since the
overflow handling depended on undefined behavior, a sufficiently smart
optimizing compiler would have realized that it could just ignore the
possibility of overflow and omit all the overflow handling code, leaving only
the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code
(tested with clang 3.9 with -O2).

BUG=chromium:601728
==========

[commit-bot: I haz the power, 2016-05-23 11:07:07]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-05-23 11:07:18]

Description was changed from

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are
supposed to handle overflow gracefully, and they probably did. But since the
overflow handling depended on undefined behavior, a sufficiently smart
optimizing compiler would have realized that it could just ignore the
possibility of overflow and omit all the overflow handling code, leaving only
the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code
(tested with clang 3.9 with -O2).

BUG=chromium:601728
==========

to

==========
Fix an UBSan error (signed overflow) in saturating addition and subtraction

Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are
supposed to handle overflow gracefully, and they probably did. But since the
overflow handling depended on undefined behavior, a sufficiently smart
optimizing compiler would have realized that it could just ignore the
possibility of overflow and omit all the overflow handling code, leaving only
the unadorned addition or subtraction.

Also, the new implementations, unlike the old ones, result in branch-free code
(tested with clang 3.9 with -O2).

BUG=chromium:601728

Committed: https://crrev.com/bca568bfc59f944335510762f47fe4414ea1273e
Cr-Commit-Position: refs/heads/master@{#12846}
==========

[commit-bot: I haz the power, 2016-05-23 11:07:19]

Patchset 2 (id:??) landed as
https://crrev.com/bca568bfc59f944335510762f47fe4414ea1273e
Cr-Commit-Position: refs/heads/master@{#12846}