| Summary: | [GTK] Rounding errors on 32-bit machines causes tests to fail | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | vanuan | ||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | mrobinson, pnormand, zimmermann | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Bug Depends on: | 75846 | ||||||||
| Bug Blocks: | 54474, 62204 | ||||||||
| Attachments: |
|
||||||||
Great that you're working on this as well. It turns out there are still numerical instabilities, especially in SVG <path> dumping, and in the expected.txt files where it says "RenderFoo at (0.00,1.00) size 100x100". Both use String::format, once directly, once through TextStream::operator<<(float). I'm working on a fix for that and like to see the impact on Gtk before this gets landed, if possible. I'll try to finish it in the next (few) hour(s). (In reply to comment #0) > How can we pass cppflags to autogen scripts from Tools/Scripts/webkitdirs.pm ? You can do something like: CXXFLAGS=... ./autogen.sh See also the help of the configure script. Another thing to consider for you patch is that we've skipped many tests because of these rounding errors. It probably be good to unskip them all and generate new results. Some of them will probably fail again, but we can reskip them after you patch lands. To find the skipped tests just open LayoutTests/platform/gtk/Skipped and search for the phrase "rounding". Again, I really appreciate you solving this puzzle! (In reply to comment #3) > Another thing to consider for you patch is that we've skipped many tests because of these rounding errors. It probably be good to unskip them all and generate new results. Some of them will probably fail again, but we can reskip them after you patch lands. > > To find the skipped tests just open LayoutTests/platform/gtk/Skipped and search for the phrase "rounding". This is potentially fixed by r101342, we need to unskip it and try all the currently skipped tests w/o the "workaround" suggested by altering the compile flags. (In reply to comment #4) > (In reply to comment #3) > > Another thing to consider for you patch is that we've skipped many tests because of these rounding errors. It probably be good to unskip them all and generate new results. Some of them will probably fail again, but we can reskip them after you patch lands. > > > > To find the skipped tests just open LayoutTests/platform/gtk/Skipped and search for the phrase "rounding". > > This is potentially fixed by r101342, we need to unskip it and try all the currently skipped tests w/o the "workaround" suggested by altering the compile flags. We have a new list of svg and tables tests skipped, even after r101342. Can we give this patch a try? Created attachment 119402 [details]
proposed patch
A clean build should be needed I think.
(In reply to comment #6) > Created an attachment (id=119402) [details] > proposed patch > > A clean build should be needed I think. I got the current unskipped svg tests passing on my 32-bit Release build with this patch. Created attachment 119409 [details]
svg rebaseline
Comment on attachment 119402 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=119402&action=review Thank you! > Tools/Scripts/webkitdirs.pm:1627 > + # used on Chromium build. > + $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse"; I think we should only pass these flags if we are on a 32-bit system. View in context: https://bugs.webkit.org/attachment.cgi?id=119402&action=review >> Tools/Scripts/webkitdirs.pm:1627 >> + $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse"; > > I think we should only pass these flags if we are on a 32-bit system. And I think you should append these flags, not overwrite. Sorry for not commenting earlier, really busy these days. I think this is probably the right way to go, it's truly hard to find the real cause of the rounding diffs. Let's try if the bots like it :-) (In reply to comment #10) > View in context: https://bugs.webkit.org/attachment.cgi?id=119402&action=review > > >> Tools/Scripts/webkitdirs.pm:1627 > >> + $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse"; > > > > I think we should only pass these flags if we are on a 32-bit system. > > And I think you should append these flags, not overwrite. They're appended, AFAIK. Comment on attachment 119409 [details]
svg rebaseline
Will land this once the bots cycled the patch.
Committed r103040: <http://trac.webkit.org/changeset/103040> But wait:
if ($architecture ne "x86_64") {
doesn't this mean that only two architectures are supported? What about ARM?
And what if I want to crosscompile 32 bit webkit on 64 bit machine? (In reply to comment #16) > And what if I want to crosscompile 32 bit webkit on 64 bit machine? This change is mostly meant for our buildbots. And we have no ARM bot, so far. In any case you can still do something like: CXXFLAGS="..." build-webkit ... Or tweak webkitdirs.pm again. > And we have no ARM bot, so far. Somebody already has: https://bugs.webkit.org/show_bug.cgi?id=75846 Does this patch affect only buildbot? And developers will still have these failures? I thought development builds and buildbot builds should be exactly the same? If so, what's the point of buildbot then if developers will get a different results? (In reply to comment #18) > Does this patch affect only buildbot? If you are building a development release, you should use configure and make manually to compile. This change only affects build-webkit, which is used by developers working on trunk, the build bots and the EWS. > And developers will still have these failures? Anyone not compiling with build-webkit will see these failures. These are only "failures" in the sense that they cause the layout tests to fail, as they are rounding errors that typically result in one pixel differences. This is not something that matters or would be noticed when actually using WebKit. On the other hand, these tests probably fail on ARM anyway, if rounding affects the results. > I thought development builds and buildbot builds should be exactly the same? They are not. The build bot often tests features that are unfinished or experimental, but we want test coverage for. > If so, what's the point of buildbot then if developers will get a different results? The build bots typically test a superset of the features in development and stable releases. In this case, it was appropriate to add these flags only for build-webkit, because they improve result consistency at the expense of some unknown performance impact. We didn't want this work-around to affect the performance of releases. *** Bug 39022 has been marked as a duplicate of this bug. *** |
Many SVG tests are failing on 32 bit Gtk build. I've managed to have all rounding related tests pass by using these additional flags: Index: GNUmakefile.am =================================================================== --- GNUmakefile.am (revision 99472) +++ GNUmakefile.am (working copy) @@ -116,7 +116,8 @@ -Wformat -Wformat-security -Wno-format-y2k -Wundef \ -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings \ -Wno-unused-parameter -Wno-parentheses \ - -fno-exceptions -DENABLE_GLIB_SUPPORT=1 + -fno-exceptions -DENABLE_GLIB_SUPPORT=1 \ + -march=pentium4 -msse2 -mfpmath=sse global_cxxflags += \ The description of these flags and why they are needed is here: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/build/common.gypi&q=file:build/common.gypi&exact_package=chromium&l=1718 Martin Robinson suggested that I should modify Tools/Scripts/webkitdirs.pm to ensure that this work-around only be applied when building via build-webkit. But I'm not sure how to do that. How can we pass cppflags to autogen scripts from Tools/Scripts/webkitdirs.pm ?