Now that we use a custom type for layout sizes and locations it is quite easy to overload the operators to support saturation arithmetic. Add an experimental implementation, guarded by a flag.
Created attachment 159167 [details] Patch
Comment on attachment 159167 [details] Patch Attachment 159167 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13517877 New failing tests: css1/formatting_model/inline_elements.html fast/html/details-add-summary-1.html fast/html/details-add-details-child-1.html fast/html/details-add-child-1.html fast/forms/datalist/input-appearance-range-with-datalist-zoomed.html fast/html/details-add-summary-4.html fast/frames/cached-frame-counter.html fast/borders/bidi-012.html fast/encoding/utf-16-little-endian.html fast/html/details-add-summary-2.html fast/html/details-add-summary-3-and-click.html fast/canvas/webgl/shader-precision-format.html fast/html/details-add-child-2.html fast/forms/007.html fast/block/margin-collapse/103.html fast/forms/date/date-appearance.html fast/borders/border-radius-huge-assert.html fast/html/details-add-details-child-2.html fast/flexbox/flex-hang.html fast/html/details-add-summary-1-and-click.html fast/html/details-add-summary-2-and-click.html fast/css/bidi-override-in-anonymous-block.html fast/encoding/utf-16-big-endian.html fast/backgrounds/background-position-parsing.html fast/borders/bidi-009a.html fast/html/details-add-summary-4-and-click.html fast/html/details-add-summary-3.html
Created attachment 159200 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 159489 [details] Patch
Levi Sir! very friendly review ping since you guys are our defacto FractionalLayoutUnit experts. We are pretty excited for this change, since it will fix many of Use-after-free bugs caused in float due to these overflows and wraps. e.g. https://bugs.webkit.org/show_bug.cgi?id=93135
Comment on attachment 159489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159489&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:66 > -const int intMinForLayoutUnit = -intMaxForLayoutUnit; > +const int intMinForLayoutUnit = INT_MIN / kFixedPointDenominator; A comment in the changelog is warranted. This isn't directly related to the change at hand. > Source/WebCore/platform/FractionalLayoutUnit.h:78 > + FractionalLayoutUnit(int value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } > + FractionalLayoutUnit(unsigned short value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } > + FractionalLayoutUnit(unsigned int value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } Why not have the REPORT_OVERFLOW functionality in setValue? Also, I don't think it makes sense to report anything if we're clamping. In that case there's nothing to report! > Source/WebCore/platform/FractionalLayoutUnit.h:470 > + return boundedMultiply(a, b); Perhaps we should rename this to saturatedMultiply for consistency?
Created attachment 159759 [details] Patch
(In reply to comment #6) > (From update of attachment 159489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159489&action=review > > > Source/WebCore/platform/FractionalLayoutUnit.h:66 > > -const int intMinForLayoutUnit = -intMaxForLayoutUnit; > > +const int intMinForLayoutUnit = INT_MIN / kFixedPointDenominator; > > A comment in the changelog is warranted. This isn't directly related to the change at hand. Done. > > > Source/WebCore/platform/FractionalLayoutUnit.h:78 > > + FractionalLayoutUnit(int value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } > > + FractionalLayoutUnit(unsigned short value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } > > + FractionalLayoutUnit(unsigned int value) { REPORT_OVERFLOW(isInBounds(value)); setValue(value); } > > Why not have the REPORT_OVERFLOW functionality in setValue? > > Also, I don't think it makes sense to report anything if we're clamping. In that case there's nothing to report! Done. > > > Source/WebCore/platform/FractionalLayoutUnit.h:470 > > + return boundedMultiply(a, b); > > Perhaps we should rename this to saturatedMultiply for consistency? Kept it as saturatedMultiply for now as it, unlike the saturatedAddition/Subtraction methods, doesn't necessarily use saturation arithmetic (guarded by a different flag) and takes a different type.
Comment on attachment 159759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159759&action=review I would prefer having tests for WTF. > Source/WTF/ChangeLog:12 > + * wtf/MathExtras.h: > + (saturatedAddition): > + (saturatedSubtraction): Is this better suited for CheckedArithmetic.h? > Source/WTF/wtf/MathExtras.h:381 > +{ > + uint32_t ua = a; > + uint32_t ub = b; > + uint32_t result = ua + ub; > + > + // Can only overflow if the signed bit of the two values match. If the signed > + // bit of the result and one of the values differ it did overflow. > + if (!((ua ^ ub) >> 31) && (result ^ ua) >> 31) > + result = std::numeric_limits<int>::max() + (ua >> 31); > + > + return result; I am confused by this. If I do saturatedAddition(0, -1), the result would be 1?.
Created attachment 159773 [details] Test case
(In reply to comment #9) > Is this better suited for CheckedArithmetic.h? Perhaps, it is not in the Checked namespace though. > > Source/WTF/wtf/MathExtras.h:381 > > +{ > > + uint32_t ua = a; > > + uint32_t ub = b; > > + uint32_t result = ua + ub; > > + > > + // Can only overflow if the signed bit of the two values match. If the signed > > + // bit of the result and one of the values differ it did overflow. > > + if (!((ua ^ ub) >> 31) && (result ^ ua) >> 31) > > + result = std::numeric_limits<int>::max() + (ua >> 31); > > + > > + return result; > > I am confused by this. > > If I do saturatedAddition(0, -1), the result would be 1?. No, it'll be -1. See the attached test case.
The test case should go in WebKitTestAPI. I know it is not ideal, but it is better than having it outside WebKit.
(In reply to comment #12) > The test case should go in WebKitTestAPI. > I know it is not ideal, but it is better than having it outside WebKit. Better than outside the source tree for sure. I'll convert it to a proper test and move it there. Thanks.
Comment on attachment 159759 [details] Patch > > If I do saturatedAddition(0, -1), the result would be 1?. > > No, it'll be -1. See the attached test case. Of course, my bad. For some reason I thought the return was unsigned or something... I r- for the tests and because IMHO, this does not belong in MathExtras.h (maybe CheckedArithmetic.h or a new SaturatedArithmetic.h?).
MathExtras.h is supposed to repair and slightly supplement <math.h>. So I agree that these functions should go elsewhere.
Created attachment 159829 [details] Patch
Thanks for the review and the suggestions Benjamin. I moved the functions to a new SaturatedArithmetic.h file and added tests in WebKitTestAPI, please let me know what you think.
Comment on attachment 159829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159829&action=review > Source/WTF/wtf/SaturatedArithmetic.h:46 > + if (!((ua ^ ub) >> 31) && (result ^ ua) >> 31) Use single & if you want to reduce branching. > Source/WTF/wtf/SaturatedArithmetic.h:60 > + if ((ua ^ ub) >> 31 && (result ^ ua) >> 31) ditto > Source/WebCore/platform/FractionalLayoutUnit.h:84 > + m_value = std::min<float>(std::max<float>(value * kFixedPointDenominator, intMinForLayoutUnit), intMaxForLayoutUnit); clampTo from MathExtras.h? > Source/WebCore/platform/FractionalLayoutUnit.h:93 > + m_value = std::min<double>(std::max<double>(value * kFixedPointDenominator, intMinForLayoutUnit), intMaxForLayoutUnit); ditto > Source/WebCore/platform/FractionalLayoutUnit.h:112 > + v.m_value = std::min<double>(std::max<double>(ceilf(value * kFixedPointDenominator), INT_MIN), INT_MAX); clampToInteger > Source/WebCore/platform/FractionalLayoutUnit.h:124 > + v.m_value = std::min<double>(std::max<double>(floorf(value * kFixedPointDenominator), INT_MIN), INT_MAX); ditto
(In reply to comment #18) > (From update of attachment 159829 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159829&action=review > > > Source/WTF/wtf/SaturatedArithmetic.h:46 > > + if (!((ua ^ ub) >> 31) && (result ^ ua) >> 31) > > Use single & if you want to reduce branching. Of course, thank you for catching that. > > Source/WebCore/platform/FractionalLayoutUnit.h:84 > > + m_value = std::min<float>(std::max<float>(value * kFixedPointDenominator, intMinForLayoutUnit), intMaxForLayoutUnit); > > clampTo from MathExtras.h? Good idea, changed here and elsewhere.
Created attachment 159955 [details] Patch
Comment on attachment 159955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159955&action=review Looks good once the REPORT_OVERFLOW corrections are made. I'm excited to see how this fares in the PLT. It'd be great to be able to turn this on... > Source/WebCore/platform/FractionalLayoutUnit.h:82 > + REPORT_OVERFLOW(isInBounds(value)); REPORT_OVERFLOW should only trigger without saturated arithmetic. > Source/WebCore/platform/FractionalLayoutUnit.h:91 > + REPORT_OVERFLOW(isInBounds(value)); Ditto. > Source/WebCore/platform/FractionalLayoutUnit.h:109 > REPORT_OVERFLOW(isInBounds(value)); Ditto. > Source/WebCore/platform/FractionalLayoutUnit.h:142 > static FractionalLayoutUnit fromFloatRound(float value) > { > +#if ENABLE(SATURATED_LAYOUT_ARITHMETIC) > + if (value >= 0) > + return clamp(value + epsilon() / 2.0f); > + return clamp(value - epsilon() / 2.0f); > +#else > if (value >= 0) > return FractionalLayoutUnit(value + epsilon() / 2.0f); > return FractionalLayoutUnit(value - epsilon() / 2.0f); > +#endif > } This function needs a REPORT_OVERFLOW for the non-saturated case.
Comment on attachment 159955 [details] Patch Some comments (the patch looks good otherwise): > Source/WTF/WTF.xcodeproj/project.pbxproj:1068 > + 14F3B0F715E45E4600210069 /* SaturatedArithmetic.h in Headers */, It would be great if you could order the build section alphabetically. It helps a lot when there are merge conflicts with your local changes. > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:979 > + 14F3B11315E45EAB00210069 /* SaturatedArithmeticOperations.cpp in Sources */, ditto.
Created attachment 160176 [details] Patch
Made the ASSERT changed levi suggested and sorted the xcode project files. Please take another look Benjamin.
Comment on attachment 160176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160176&action=review Thank you for fixing the xcodeproj files. Few little comments bellow, but otherwise this looks correct. > Source/WTF/ChangeLog:16 > + * GNUmakefile.list.am: > + * WTF.gypi: > + * WTF.pro: > + * WTF.vcproj/WTF.vcproj: > + * WTF.xcodeproj/project.pbxproj: Not the CMakeLists.txt? > Source/WTF/wtf/Platform.h:835 > +#if !defined(ENABLE_SATURATED_LAYOUT_ARITHMETIC) > +#define ENABLE_SATURATED_LAYOUT_ARITHMETIC 0 > +#endif > + > +#if ENABLE(ENABLE_SATURATED_LAYOUT_ARITHMETIC) && !ENABLE(ENABLE_SUBPIXEL_LAYOUT) > +#error "ENABLE_SATURATED_LAYOUT_ARITHMETIC requires ENABLE_SUBPIXEL_LAYOUT" > +#endif Just, please, please, please, if that ends up being unused, please remove the code. We have so much dead code around already. :( > Source/WTF/wtf/SaturatedArithmetic.h:47 > + // Can only overflow if the signed bit of the two values match. If the signed > + // bit of the result and one of the values differ it did overflow. > + if (!((ua ^ ub) >> 31) & (result ^ ua) >> 31) > + result = std::numeric_limits<int>::max() + (ua >> 31); I have the feeling we can do better with assembly and jump-if-overflow. But for now that is a good start. :) > Source/WTF/wtf/SaturatedArithmetic.h:61 > + if ((ua ^ ub) >> 31 & (result ^ ua) >> 31) > + result = std::numeric_limits<int>::max() + (ua >> 31); Ditto. > Source/WebCore/platform/FractionalLayoutUnit.h:67 > -const int intMinForLayoutUnit = -intMaxForLayoutUnit; > +const int intMinForLayoutUnit = INT_MIN / kFixedPointDenominator; This looks correct but can't it be tested? > Source/WebCore/platform/FractionalLayoutUnit.h:138 > + REPORT_OVERFLOW(isInBounds(value)); Should't this be?: REPORT_OVERFLOW(isInBounds(value + epsilon())); REPORT_OVERFLOW(isInBounds(value - epsilon())); > Source/WebCore/platform/FractionalLayoutUnit.h:274 > + REPORT_OVERFLOW(isInBounds(value)); Not sure how that is even correct but that's the original code so.... Value can be in bounds and still have value * kFixedPointDenominator overflow. > Source/WebCore/platform/FractionalLayoutUnit.h:286 > + REPORT_OVERFLOW(isInBounds(value)); Ditto.
Thank you, I'll address your concerns before committing. (In reply to comment #25) > Just, please, please, please, if that ends up being unused, please remove the code. > We have so much dead code around already. :( Of course.
Created attachment 160229 [details] Patch for landing
Comment on attachment 160229 [details] Patch for landing Clearing flags on attachment: 160229 Committed r126509: <http://trac.webkit.org/changeset/126509>
All reviewed patches have been landed. Closing bug.