Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774963002:
work around leftshift for negative values (Closed)
Descriptionwork around leftshift for negative values
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1774963002
Committed: https://skia.googlesource.com/skia/+/aa5e1ae06b33841ad4efc9fbf4c8b2cd6236d58b
Patch Set 1 #
Total comments: 2
Messages
Total messages: 24 (11 generated)
Description was changed from ========== work around leftshift for negative values BUG=skia: ========== to ========== work around leftshift for negative values BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + caryclark@google.com, robertphillips@google.com
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774963002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
https://codereview.chromium.org/1774963002/diff/1/src/core/SkEdge.cpp File src/core/SkEdge.cpp (right): https://codereview.chromium.org/1774963002/diff/1/src/core/SkEdge.cpp#newcode331 src/core/SkEdge.cpp:331: SkFDot6 twoThird = (a + 6*b - c*15 + d*8) * 19 >> 9; I'm curious how this works. If the bug is that the highest number bit is shifted into the sign bit by the shift of a, b, c, or d, doesn't this imply that the number may be so large that it can't be safely multiplied by 19?
https://codereview.chromium.org/1774963002/diff/1/src/core/SkEdge.cpp File src/core/SkEdge.cpp (right): https://codereview.chromium.org/1774963002/diff/1/src/core/SkEdge.cpp#newcode331 src/core/SkEdge.cpp:331: SkFDot6 twoThird = (a + 6*b - c*15 + d*8) * 19 >> 9; On 2016/03/08 14:20:23, caryclark wrote: > I'm curious how this works. If the bug is that the highest number bit is shifted > into the sign bit by the shift of a, b, c, or d, doesn't this imply that the > number may be so large that it can't be safely multiplied by 19? I don't think that is the bug. I think the bug from ASAN is that one of the values was negative at all (as that is "undefined"), not that we actually overflowed.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
Correct, left shifting any negative value is undefined (but often totally sensible, which is why we added SkLeftShift). _Hopefully_ if this CL were just converting overflow-by-shift to overflow-by-multiplication, we'd see the ASAN bot complain about that too: it's got the signed-integer-overflow sanitizer turned on as well.
lgtm
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774963002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774963002/1
The CQ bit was unchecked by mtklein@chromium.org
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774963002/1
Message was sent while issue was closed.
Description was changed from ========== work around leftshift for negative values BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== work around leftshift for negative values BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/aa5e1ae06b33841ad4efc9fbf4c8b2cd6236d58b ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/aa5e1ae06b33841ad4efc9fbf4c8b2cd6236d58b
Message was sent while issue was closed.
In my zeal to speed up the CQ, I disabled nanobench on Debug CPU trybots. This means the ASAN trybot is not currently useful to see if we fixed this bug. I'm going to land the CL, and take a look at how to focus down that disabled-nanobench feature to not apply to *SANs. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
