Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1999653002:
Fix UBSan errors (left shift on negative value) (Closed)
|
Created:
4 years, 7 months ago by minyue-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix UBSan errors (left shift on negative value)
BUG=webrtc:5530
Committed: https://crrev.com/30629957e18ce563331b714e01b03e5c08b62de7
Cr-Commit-Position: refs/heads/master@{#12820}
Patch Set 1 #
Total comments: 11
Patch Set 2 : refinement #Messages
Total messages: 25 (12 generated)
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999653002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999653002/1
Description was changed from ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 ========== to ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 ==========
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
Hi Karl, You dealt with similar issues recently. Could you help in reviewing this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but see comments https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/digital_agc.c (right): https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:121: kLog10_2 / 2); Yes, replacing x << S with x * (1 << S) is what I ended up doing after trying out some alternatives. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:147: inLevel = ((int32_t)diffGain * (1 << 14)) - inLevel; // Q14 You can remove the outer layer of parentheses here, since (cast) binds tighter than *, which binds tighter than -. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:189: numFIX = (maxGain * constMaxGain) * 64; // Q14 It's probably better to write it as * (1 << 6), since that emphasizes that it's just a left shift. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:202: numFIX *= (1 << zeros); // Q(14+zeros) Optionally remove the parentheses. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:564: gain32 = gains[0] * 16; Again, 1<<4 is arguably more readable. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:592: gain32 = gains[k] * 16; * (1 << 4)
thanks for review! all fixed. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/digital_agc.c (right): https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:147: inLevel = ((int32_t)diffGain * (1 << 14)) - inLevel; // Q14 On 2016/05/20 07:05:56, kwiberg-webrtc wrote: > You can remove the outer layer of parentheses here, since (cast) binds tighter > than *, which binds tighter than -. Done. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:189: numFIX = (maxGain * constMaxGain) * 64; // Q14 On 2016/05/20 07:05:56, kwiberg-webrtc wrote: > It's probably better to write it as * (1 << 6), since that emphasizes that it's > just a left shift. Done. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:202: numFIX *= (1 << zeros); // Q(14+zeros) On 2016/05/20 07:05:56, kwiberg-webrtc wrote: > Optionally remove the parentheses. Done. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:564: gain32 = gains[0] * 16; On 2016/05/20 07:05:56, kwiberg-webrtc wrote: > Again, 1<<4 is arguably more readable. Done. https://codereview.webrtc.org/1999653002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/digital_agc.c:592: gain32 = gains[k] * 16; On 2016/05/20 07:05:56, kwiberg-webrtc wrote: > * (1 << 4) Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1999653002/#ps20001 (title: "refinement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999653002/20001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5670)
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
On 2016/05/20 07:30:16, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5670) +henrik for owner's approval
rubberstamp lgtm
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999653002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 ========== to ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 ========== to ========== Fix UBSan errors (left shift on negative value) BUG=webrtc:5530 Committed: https://crrev.com/30629957e18ce563331b714e01b03e5c08b62de7 Cr-Commit-Position: refs/heads/master@{#12820} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/30629957e18ce563331b714e01b03e5c08b62de7 Cr-Commit-Position: refs/heads/master@{#12820}
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted |
