Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128633003:
pathops coincidence and security rewrite (Closed)
Descriptionpathops coincidence and security rewrite
Most changes stem from working on an examples bracketed
by #if DEBUG_UNDER_DEVELOPMENT // tiger
These exposed many problems with coincident curves,
as well as errors throughout the code.
Fixing these errors also fixed a number of fuzzer-inspired
bug reports.
* Line/Curve Intersections
Check to see if the end of the line nearly intersects
the curve. This was a FIXME in the old code.
* Performance
Use a central chunk allocator.
Plumb the allocator into the global variable state
so that it can be shared. (Note that 'SkGlobalState'
is allocated on the stack and is visible to children
functions but not other threads.)
* Refactor
Let SkOpAngle grow up from a structure to a class.
Let SkCoincidentSpans grow up from a structure to a class.
Rename enum Alias to AliasMatch.
* Coincidence Rewrite
Add more debugging to coincidence detection.
Parallel debugging routines have read-only logic to report
the current coincidence state so that steps through the
logic can expose whether things got better or worse.
More functions can error-out and cause the pathops
engine to non-destructively exit.
* Accuracy
Remove code that adjusted point locations. Instead,
offset the curve part so that sorted curves all use
the same origin.
Reduce the size (and influence) of magic numbers.
* Testing
The debug suite with verify and the full release suite
./out/Debug/pathops_unittest -v -V
./out/Release/pathops_unittest -v -V -x
expose one error. That error is captured as cubics_d3.
This error exists in the checked in code as well.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003
Committed: https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17
Patch Set 1 #Patch Set 2 : fix more unit tests #Patch Set 3 : completes test, still flaky #Patch Set 4 : add test case for flake #Patch Set 5 : add dean 4 test #Patch Set 6 : fix most cubic normal fails #Patch Set 7 : loosen close tolerance to get through tests #Patch Set 8 : isolate broken test case #Patch Set 9 : working on testQuads67 #Patch Set 10 : try contains instead of equal #Patch Set 11 : rough check experiment #Patch Set 12 : tracking down cubics_d3 bug #Patch Set 13 : mark failures to investigate #Patch Set 14 : fix remaining tests #Patch Set 15 : whoops; disabled test expectation checks #Patch Set 16 : turn tabs into spaces #Patch Set 17 : one more tab fix, obsolete comment #Patch Set 18 : scale big numbers down #Patch Set 19 : wip; update debug versions #Patch Set 20 : updated debug versions #Patch Set 21 : fix error; remove trailing spaces #Patch Set 22 : require resulting t to be between 0 and 1 #
Total comments: 22
Patch Set 23 : address comments #Patch Set 24 : disable new test since it fails on skia_fast #Messages
Total messages: 30 (22 generated)
Description was changed from ========== merged coin op fixes This is a mashup of changes described in 1828383002. Some additional changes were required to merge with ToT. Additional bug fixes made when verifying against full test suite. BUG=skia: ========== to ========== merged coin op fixes This is a mashup of changes described in 1828383002. Some additional changes were required to merge with ToT. Additional bug fixes made when verifying against full test suite. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Mac-Clang-x86_64-Release-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
Description was changed from ========== merged coin op fixes This is a mashup of changes described in 1828383002. Some additional changes were required to merge with ToT. Additional bug fixes made when verifying against full test suite. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ========== to ========== pathops coincidence and security rewrite * Line/Curve Intersections Check to see if the end of the line nearly intersects the curve. This was a FIXME in the old code. * Performance Use a central chunk allocator. Plumb the allocator into the global variable state so that it can be shared. (Note that 'SkGlobalState' is allocated on the stack and is visible to children functions but not other threads.) * Refactor Let SkOpAngle grow up from a structure to a class. Let SkCoincidentSpans grow up from a structure to a class. Rename enum Alias to AliasMatch. * Coincidence Rewrite Add more debugging to coincidence detection. Parallel debugging routines have read-only logic to report the current coincidence state so that steps through the logic can expose whether things got better or worse. More functions can error-out and cause the pathops engine to non-destructively exit. * Accuracy Remove code that adjusted point locations. Instead, offset the curve part so that sorted curves all use the same origin. Reduce the size (and influence) of magic numbers. This is a mashup of changes described in 1828383002. Some additional changes were required to merge with ToT. Additional bug fixes made when verifying against full test suite. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== pathops coincidence and security rewrite * Line/Curve Intersections Check to see if the end of the line nearly intersects the curve. This was a FIXME in the old code. * Performance Use a central chunk allocator. Plumb the allocator into the global variable state so that it can be shared. (Note that 'SkGlobalState' is allocated on the stack and is visible to children functions but not other threads.) * Refactor Let SkOpAngle grow up from a structure to a class. Let SkCoincidentSpans grow up from a structure to a class. Rename enum Alias to AliasMatch. * Coincidence Rewrite Add more debugging to coincidence detection. Parallel debugging routines have read-only logic to report the current coincidence state so that steps through the logic can expose whether things got better or worse. More functions can error-out and cause the pathops engine to non-destructively exit. * Accuracy Remove code that adjusted point locations. Instead, offset the curve part so that sorted curves all use the same origin. Reduce the size (and influence) of magic numbers. This is a mashup of changes described in 1828383002. Some additional changes were required to merge with ToT. Additional bug fixes made when verifying against full test suite. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ========== to ========== pathops coincidence and security rewrite Most changes stem from working on an examples bracketed by #if DEBUG_UNDER_DEVELOPMENT // tiger These exposed many problems with coincident curves, as well as errors throughout the code. Fixing these errors also fixed a number of fuzzer-inspired bug reports. * Line/Curve Intersections Check to see if the end of the line nearly intersects the curve. This was a FIXME in the old code. * Performance Use a central chunk allocator. Plumb the allocator into the global variable state so that it can be shared. (Note that 'SkGlobalState' is allocated on the stack and is visible to children functions but not other threads.) * Refactor Let SkOpAngle grow up from a structure to a class. Let SkCoincidentSpans grow up from a structure to a class. Rename enum Alias to AliasMatch. * Coincidence Rewrite Add more debugging to coincidence detection. Parallel debugging routines have read-only logic to report the current coincidence state so that steps through the logic can expose whether things got better or worse. More functions can error-out and cause the pathops engine to non-destructively exit. * Accuracy Remove code that adjusted point locations. Instead, offset the curve part so that sorted curves all use the same origin. Reduce the size (and influence) of magic numbers. * Testing The debug suite with verify and the full release suite ./out/Debug/pathops_unittest -v -V ./out/Release/pathops_unittest -v -V -x expose one error. That error is captured as cubics_d3. This error exists in the checked in code as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
caryclark@google.com changed reviewers: + herb@google.com
I did not follow most of the new coincident logic. I assume this is vetted by the new test cases? https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDConicLi... File src/pathops/SkDConicLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDConicLi... src/pathops/SkDConicLineIntersection.cpp:244: addLineNearEndPoints(); this->? https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDCubicLi... File src/pathops/SkDCubicLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDCubicLi... src/pathops/SkDCubicLineIntersection.cpp:336: addLineNearEndPoints(); this-> https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDQuadLin... File src/pathops/SkDQuadLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDQuadLin... src/pathops/SkDQuadLineIntersection.cpp:342: addLineNearEndPoints(); this-> https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... File src/pathops/SkOpCoincidence.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... src/pathops/SkOpCoincidence.cpp:28: void (SkCoincidentSpans::*setEnd)(const SkOpPtT* ptT) ) { Do you need method pointers? Can this just use plan class style polymorphism? Or use std::function and pass in a lambda. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... src/pathops/SkOpCoincidence.cpp:237: // FIXME: caller should have already sorted Does this need an assert to check for sortedness? https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... File src/pathops/SkPathOpsCurve.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... src/pathops/SkPathOpsCurve.cpp:72: void SkDCurve::setCubicBounds(const SkPoint curve[4], SkScalar , Space before "," https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... File src/pathops/SkPathOpsCurve.h (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... src/pathops/SkPathOpsCurve.h:137: static SkDPoint (* const CurveDDPointAtT[])(const SkDCurve& , double ) = { Space before "," https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... File src/pathops/SkPathOpsDebug.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:768: if (span && span->segment() != opp && span->containsCoincidence(opp)) { // debug has additional condition since it may be called before inner duplicate points have been deleted Comments on next line? https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:817: // SkDebugf("%s coinSpan=%d endSpan=%d oppSpan=%d oppEndSpan=%d\n", __FUNCTION__, Remove commented out code. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:956: // return; Remove commented out code. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:1495: // cs = csWritable; You have a bunch of commented out code in this routine. Is it still needed?
https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDConicLi... File src/pathops/SkDConicLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDConicLi... src/pathops/SkDConicLineIntersection.cpp:244: addLineNearEndPoints(); On 2016/07/18 15:13:38, herb_g wrote: > this->? Done. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDCubicLi... File src/pathops/SkDCubicLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDCubicLi... src/pathops/SkDCubicLineIntersection.cpp:336: addLineNearEndPoints(); On 2016/07/18 15:13:39, herb_g wrote: > this-> Done. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDQuadLin... File src/pathops/SkDQuadLineIntersection.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkDQuadLin... src/pathops/SkDQuadLineIntersection.cpp:342: addLineNearEndPoints(); On 2016/07/18 15:13:39, herb_g wrote: > this-> Done. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... File src/pathops/SkOpCoincidence.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... src/pathops/SkOpCoincidence.cpp:28: void (SkCoincidentSpans::*setEnd)(const SkOpPtT* ptT) ) { On 2016/07/18 15:13:39, herb_g wrote: > Do you need method pointers? Can this just use plan class style polymorphism? Or > use std::function and pass in a lambda. It could be rewritten. I'm curious how that rewrite will help. The disassembly looks efficient since class SkCoincidentSpans has no virtual members. Are class members inherently inefficient / thread unsafe / ? https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkOpCoinci... src/pathops/SkOpCoincidence.cpp:237: // FIXME: caller should have already sorted On 2016/07/18 15:13:39, herb_g wrote: > Does this need an assert to check for sortedness? If it is already sorted, the Ordered() function will do nothing. This should labeled OPTIMIZE instead of FIXME to remind me to investigate if there is a worthwhile performance improvement to be had here. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... File src/pathops/SkPathOpsCurve.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... src/pathops/SkPathOpsCurve.cpp:72: void SkDCurve::setCubicBounds(const SkPoint curve[4], SkScalar , On 2016/07/18 15:13:39, herb_g wrote: > Space before "," See earlier reply. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... File src/pathops/SkPathOpsCurve.h (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsC... src/pathops/SkPathOpsCurve.h:137: static SkDPoint (* const CurveDDPointAtT[])(const SkDCurve& , double ) = { On 2016/07/18 15:13:39, herb_g wrote: > Space before "," The Skia style guide https://skia.org/dev/contrib/style doesn't provide any guidance on this. I prefer the space to remind myself that there is an unnamed parameter. If the rest of Skia is consistent about this, I should change my usage in many places. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... File src/pathops/SkPathOpsDebug.cpp (right): https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:768: if (span && span->segment() != opp && span->containsCoincidence(opp)) { // debug has additional condition since it may be called before inner duplicate points have been deleted On 2016/07/18 15:13:39, herb_g wrote: > Comments on next line? The crazy formatting here is so that SkOpSegment::debugMissingCoincidence() will line up exactly with SkOpSegment::missingCoincidence(). There are many functions here of the form SkOpXXX::debugXXX() that follow this convention. These functions are read-only versions of the actual code to permit preflighting coincident logic. All possible code paths are executed so I can visualize the various side effects that each code path may cause. This is all bracketed by #if DEBUG_COINCIDENCE_VERBOSE https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:817: // SkDebugf("%s coinSpan=%d endSpan=%d oppSpan=%d oppEndSpan=%d\n", __FUNCTION__, On 2016/07/18 15:13:39, herb_g wrote: > Remove commented out code. See above https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:956: // return; On 2016/07/18 15:13:39, herb_g wrote: > Remove commented out code. See comment above. https://codereview.chromium.org/2128633003/diff/400001/src/pathops/SkPathOpsD... src/pathops/SkPathOpsDebug.cpp:1495: // cs = csWritable; On 2016/07/18 15:13:39, herb_g wrote: > You have a bunch of commented out code in this routine. Is it still needed? See reply above.
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
lgtm
The CQ bit was checked by caryclark@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== pathops coincidence and security rewrite Most changes stem from working on an examples bracketed by #if DEBUG_UNDER_DEVELOPMENT // tiger These exposed many problems with coincident curves, as well as errors throughout the code. Fixing these errors also fixed a number of fuzzer-inspired bug reports. * Line/Curve Intersections Check to see if the end of the line nearly intersects the curve. This was a FIXME in the old code. * Performance Use a central chunk allocator. Plumb the allocator into the global variable state so that it can be shared. (Note that 'SkGlobalState' is allocated on the stack and is visible to children functions but not other threads.) * Refactor Let SkOpAngle grow up from a structure to a class. Let SkCoincidentSpans grow up from a structure to a class. Rename enum Alias to AliasMatch. * Coincidence Rewrite Add more debugging to coincidence detection. Parallel debugging routines have read-only logic to report the current coincidence state so that steps through the logic can expose whether things got better or worse. More functions can error-out and cause the pathops engine to non-destructively exit. * Accuracy Remove code that adjusted point locations. Instead, offset the curve part so that sorted curves all use the same origin. Reduce the size (and influence) of magic numbers. * Testing The debug suite with verify and the full release suite ./out/Debug/pathops_unittest -v -V ./out/Release/pathops_unittest -v -V -x expose one error. That error is captured as cubics_d3. This error exists in the checked in code as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 ========== to ========== pathops coincidence and security rewrite Most changes stem from working on an examples bracketed by #if DEBUG_UNDER_DEVELOPMENT // tiger These exposed many problems with coincident curves, as well as errors throughout the code. Fixing these errors also fixed a number of fuzzer-inspired bug reports. * Line/Curve Intersections Check to see if the end of the line nearly intersects the curve. This was a FIXME in the old code. * Performance Use a central chunk allocator. Plumb the allocator into the global variable state so that it can be shared. (Note that 'SkGlobalState' is allocated on the stack and is visible to children functions but not other threads.) * Refactor Let SkOpAngle grow up from a structure to a class. Let SkCoincidentSpans grow up from a structure to a class. Rename enum Alias to AliasMatch. * Coincidence Rewrite Add more debugging to coincidence detection. Parallel debugging routines have read-only logic to report the current coincidence state so that steps through the logic can expose whether things got better or worse. More functions can error-out and cause the pathops engine to non-destructively exit. * Accuracy Remove code that adjusted point locations. Instead, offset the curve part so that sorted curves all use the same origin. Reduce the size (and influence) of magic numbers. * Testing The debug suite with verify and the full release suite ./out/Debug/pathops_unittest -v -V ./out/Release/pathops_unittest -v -V -x expose one error. That error is captured as cubics_d3. This error exists in the checked in code as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128633003 Committed: https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:420001) as https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17
Message was sent while issue was closed.
On 2016/07/18 17:01:43, commit-bot: I haz the power wrote: > Committed patchset #23 (id:420001) as > https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17 The failing -Fast bot is a sort of Release+ bot, built with -O3, -march=native and -fomit-frame-pointer. You can set GYP_DEFINES=skia_fast=1 to try to reproduce this failure on your own machine, though because of -march=native it may vary from machine to machine. The bots building and running this bot are most similar to a Z840. https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX...
Message was sent while issue was closed.
On 2016/07/18 17:36:03, mtklein wrote: > On 2016/07/18 17:01:43, commit-bot: I haz the power wrote: > > Committed patchset #23 (id:420001) as > > https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17 > > The failing -Fast bot is a sort of Release+ bot, built with -O3, -march=native > and -fomit-frame-pointer. You can set GYP_DEFINES=skia_fast=1 to try to > reproduce this failure on your own machine, though because of -march=native it > may vary from machine to machine. The bots building and running this bot are > most similar to a Z840. > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX... The Nexus9 bot also failed the same way: https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne... I think that makes FMA the most likely explanation. These two bots are the only two who can a+b*c with one rounding, and I think GCC is generally willing to compile code that doesn't explicitly ask for it (e.g. std::fma) into FMA operations when it notices it has the instructions available and they're at least as fast as doing it in two steps with two roundings. (IIRC, you have to explicitly ask Clang to act on these opportunities, probably because doing so silently can cause these confusing issues to crop up.)
Message was sent while issue was closed.
On 2016/07/18 20:25:58, mtklein wrote: > On 2016/07/18 17:36:03, mtklein wrote: > > On 2016/07/18 17:01:43, commit-bot: I haz the power wrote: > > > Committed patchset #23 (id:420001) as > > > > https://skia.googlesource.com/skia/+/55888e44171ffd48b591d19256884a969fe4da17 > > > > The failing -Fast bot is a sort of Release+ bot, built with -O3, -march=native > > and -fomit-frame-pointer. You can set GYP_DEFINES=skia_fast=1 to try to > > reproduce this failure on your own machine, though because of -march=native it > > may vary from machine to machine. The bots building and running this bot are > > most similar to a Z840. > > > > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX... > > The Nexus9 bot also failed the same way: > https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne... > > I think that makes FMA the most likely explanation. These two bots are the only > two who can a+b*c with one rounding, and I think GCC is generally willing to > compile code that doesn't explicitly ask for it (e.g. std::fma) into FMA > operations when it notices it has the instructions available and they're at > least as fast as doing it in two steps with two roundings. (IIRC, you have to > explicitly ask Clang to act on these opportunities, probably because doing so > silently can cause these confusing issues to crop up.) Yep! That's it. To reproduce with Clang (e.g. on a recent Mac), you'll need to add -ffp-contract=fast to the compile flags: $ CPPFLAGS=-ffp-contract=fast GYP_DEFINES="skia_fast=1" ./gyp_skia && ninja -C out/Release dm && out/Release/dm |
