Bug 52810 – [Qt] Couple of tests have different results on 64 bit and/or in debug mode compared to 32 bit and/or release mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

The following tests has different results on 64 bit and/or in debug mode:
css2.1/t09-c5526c-display-00-e.html 
svg/custom/recursive-filter.svg 
svg/custom/relative-sized-inner-svg.xhtml 
svg/custom/relative-sized-use-on-symbol.xhtml 
svg/custom/relative-sized-use-without-attributes-on-symbol.xhtml 
svg/custom/resource-invalidate-on-target-update.svg 
svg/filters/feColorMatrix-values.svg 
svg/filters/filteredImage.svg 
svg/zoom/page/zoom-mask-with-percentages.svg

diff: <attached> (same on 64 bit release, 64 bit debug, 32 bit debug)

[Csaba Osztrogonác]

Created attachment 79592 [details]
diff

[Csaba Osztrogonác]

similar bug: https://bugs.webkit.org/show_bug.cgi?id=52812

[Csaba Osztrogonác]

[Qt] 6 tests have different results on 64 bit compared to 32 bit

[Csaba Osztrogonác]

(In reply to comment #3)
> [Qt] 6 tests have different results on 64 bit compared to 32 bit

accidentally comment, please ignore it.

[Csaba Osztrogonác]

I added all failing tests to the Skipped list to make buildbots happy:
http://trac.webkit.org/changeset/83565

The list of failing tests should be updated.

[Csaba Osztrogonác]

Created attachment 89175 [details]
results for 32bit debug bot

[Csaba Osztrogonác]

Created attachment 89176 [details]
results for 64bit debug bot

[Csaba Osztrogonác]

Created attachment 89178 [details]
results for 64bit release bot

[Zoltan Herczeg]

In 32 and 64 debug the following SVG has 1px difference:

<?xml version="1.0" standalone="no"?>
<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 450 500">
    <circle cx="20" cy="20" r="40" style="stroke-width: 12px"/>
</svg>

10,11c10,11
<   RenderSVGRoot {svg} at (106,0) size 96x72
<     RenderSVGPath {circle} at (106,0) size 96x72 [fill={[type=SOLID] [color=#000000]}] [cx=20.00] [cy=20.00] [r=40.00]
---
>   RenderSVGRoot {svg} at (106,0) size 97x72
>     RenderSVGPath {circle} at (106,0) size 97x72 [fill={[type=SOLID] [color=#000000]}] [cx=20.00] [cy=20.00] [r=40.00]

[Zoltan Herczeg]

Seems SVGPreserveAspectRatio::getCTM is the culrpit. This is an insane x86+compiler specific issue.

Let's see the following division: 400000/600

as for (float)400000/(float)600 = 666.666687
as for (double)400000/(double)600 = 666.666667

However, forcing the value to "float" is not necessary happen. If the value is already stored in the correct (argument passing) register, the gcc optimizes out the "float" conversion, keeping the value as it is now.

[Zoltan Herczeg]

Created attachment 96230 [details]
patch

[Nikolas Zimmermann]

Comment on attachment 96230 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96230&action=review

Wow, does this fix all of the problems we have? We should also talk to the Gtk guys, who also skip lots of SVG tests because of that.

> LayoutTests/platform/qt/svg/W3C-SVG-1.1-SE/types-dom-05-b-expected.txt:61
> +      RenderSVGResourceMarker {marker} [id="m"] [markerUnits=strokeWidth] [ref at (0,0)] [angle=1.80]

You are updating this test, but its still skipped?

[Csaba Osztrogonác]

(In reply to comment #12)
> Wow, does this fix all of the problems we have? We should also talk to the Gtk guys, who also skip lots of SVG tests because of that.

No, only the unskippes tests. There are some rounding problem and +0.00 vs -0.00 problem.
 
> > LayoutTests/platform/qt/svg/W3C-SVG-1.1-SE/types-dom-05-b-expected.txt:61
> > +      RenderSVGResourceMarker {marker} [id="m"] [markerUnits=strokeWidth] [ref at (0,0)] [angle=1.80]
> 
> You are updating this test, but its still skipped?
Yes, because the results are correct on 32 bit, but fail on 64 bit.

[Zoltan Herczeg]

> Wow, does this fix all of the problems we have? We should also talk to the Gtk guys, who also skip lots of SVG tests because of that.

No. Now I open a master bug for such issues, and plan to send a mail to webkit-dev.

> You are updating this test, but its still skipped?

Ossy likes it, since the tests are ok, just they have 32/64 bit differences. From 34 test cases 22 are fixed.

[Nikolas Zimmermann]

Zoltan, can you revisit this patch? It would be great if you could investigate more in killing these differences.

[Csaba Osztrogonác]

*** Bug 52812 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

[Qt] 6 tests have different results on 64 bit compared to 32 bit
[Qt] 9 tests have different results on 64 bit and/or in debug mode compared to 32 bit and/or release mode

[Csaba Osztrogonác]

One more failing test: svg/custom/image-with-transform-clip-filter.svg

-      [filter="myfilter"] RenderSVGResourceFilter {filter} at (-50.30,9.00) size 603.60x492
+      [filter="myfilter"] RenderSVGResourceFilter {filter} at (-50.30,9) size 603.60x492

The original expected file was correct on 32 bit in release mode. I landed a "fix" to make it happy: https://trac.webkit.org/changeset/91753/trunk/LayoutTests/platform/qt/svg/custom/image-with-transform-clip-filter-expected.txt

But unfortunately it broke 64 bit and debug builds.

[Csaba Osztrogonác]

I checked Zoltán's patch, it still works, but expected files needs to be updated and there are some tests shoulbn't be removed from the skipped list.

The patch will fix the following tests:
svg/custom/circular-marker-reference-1.svg
svg/custom/circular-marker-reference-2.svg
svg/custom/circular-marker-reference-3.svg
svg/custom/circular-marker-reference-4.svg
svg/custom/empty-merge.svg
svg/custom/non-circular-marker-reference.svg
svg/custom/non-scaling-stroke-markers.svg
svg/custom/recursive-filter.svg
svg/custom/resource-invalidate-on-target-update.svg
svg/custom/text-rotated-gradient.svg
svg/filters/feColorMatrix-values.svg
svg/filters/filteredImage.svg
svg/W3C-SVG-1.1/painting-marker-03-f.svg
svg/W3C-SVG-1.1/struct-frag-03-t.svg

And it didn't fix the following tests: (but they are unskipped in the patch)
svg/W3C-SVG-1.1/animate-elem-06-t.svg
svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg
svg/custom/relative-sized-inner-svg.xhtml
svg/custom/relative-sized-use-on-symbol.xhtml
svg/custom/relative-sized-use-without-attributes-on-symbol.xhtml
svg/custom/use-css-events.svg
svg/text/text-align-05-b.svg
svg/zoom/page/zoom-mask-with-percentages.svg

[Csaba Osztrogonác]

Comment on attachment 96230 [details]
patch

r- now, because it is obsolete and needs to be updated

[Csaba Osztrogonác]

4 new tests fail: :-S (skipped by https://trac.webkit.org/changeset/91992)
svg/batik/masking/maskRegions.svg
svg/hixie/links/001.xml
svg/hixie/perf/007.xml
svg/hixie/viewbox/preserveAspectRatio/001.xml

[Csaba Osztrogonác]

One more failing test: svg/W3C-SVG-1.1-SE/filters-image-05-f.svg
Skipped by http://trac.webkit.org/changeset/94753

[Csaba Osztrogonác]

One more failing test: svg/filters/feColorMatrix-saturate.svg (skipped by http://trac.webkit.org/changeset/96587)

--- /ramdisk/qt-linux-64-release/build/layout-test-results/svg/filters/feColorMatrix-saturate-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/svg/filters/feColorMatrix-saturate-actual.txt 
@@ -1,7 +1,7 @@
 layer at (0,0) size 800x600
   RenderView at (0,0) size 800x600
 layer at (0,0) size 800x600
-  RenderSVGRoot {svg} at (0,0) size 250x251
+  RenderSVGRoot {svg} at (0,0) size 250x250
     RenderSVGHiddenContainer {defs} at (0,0) size 0x0
       RenderSVGResourceFilter {filter} [id="f1"] [filterUnits=objectBoundingBox] [primitiveUnits=userSpaceOnUse]
         [feColorMatrix type="SATURATE" values="-100.00"]

[Csaba Osztrogonác]

One more test fails on 64 bit because of this bug: svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html ( I skipped it to make bot happy. )

--- /ramdisk/qt-linux-64-release/build/layout-test-results/svg/zoom/page/zoom-img-preserveAspectRatio-support-1-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/svg/zoom/page/zoom-img-preserveAspectRatio-support-1-actual.txt 
@@ -49,7 +49,7 @@
                   RenderSVGRoot {svg} at (0,0) size 132x28
                     RenderSVGHiddenContainer {defs} at (0,0) size 0x0
                     RenderSVGContainer {g} at (0,0) size 132x28 [transform={m=((1.00,0.00)(0.00,1.00)) t=(-162.36,-403.29)}]
-                      RenderSVGPath {path} at (0,0) size 132x28 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#D9BB7A] [fill rule=EVEN-ODD]}] [data="M 525.714 585.219 C 525.739 685.437 444.503 766.692 344.286 766.692 C 244.068 766.692 162.833 685.437 162.857 585.219 C 162.833 485.002 244.068 403.746 344.286 403.746 C 444.503 403.746 525.739 485.002 525.714 585.219 Z"]
+                      RenderSVGPath {path} at (0,0) size 132x28 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#D9BB7A] [fill rule=EVEN-ODD]}] [data="M 525.714 585.219 C 525.714 685.419 444.486 766.648 344.286 766.648 C 244.085 766.648 162.857 685.419 162.857 585.219 C 162.857 485.019 244.085 403.791 344.286 403.791 C 444.486 403.791 525.714 485.019 525.714 585.219 Z"]
               RenderText {#text} at (0,0) size 0x0
           RenderTableRow {TR} at (0,101) size 462x40
             RenderTableCell {TH} at (67,112) size 112x17 [bgcolor=#DDDD99] [r=3 c=1 rs=1 cs=1]
@@ -66,7 +66,7 @@
                   RenderSVGRoot {svg} at (0,0) size 132x28
                     RenderSVGHiddenContainer {defs} at (0,0) size 0x0
                     RenderSVGContainer {g} at (0,0) size 132x28 [transform={m=((1.00,0.00)(0.00,1.00)) t=(-162.36,-403.29)}]
-                      RenderSVGPath {path} at (0,0) size 132x28 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#D9BB7A] [fill rule=EVEN-ODD]}] [data="M 525.714 585.219 C 525.739 685.437 444.503 766.692 344.286 766.692 C 244.068 766.692 162.833 685.437 162.857 585.219 C 162.833 485.002 244.068 403.746 344.286 403.746 C 444.503 403.746 525.739 485.002 525.714 585.219 Z"]
+                      RenderSVGPath {path} at (0,0) size 132x28 [stroke={[type=SOLID] [color=#000000]}] [fill={[type=SOLID] [color=#D9BB7A] [fill rule=EVEN-ODD]}] [data="M 525.714 585.219 C 525.714 685.419 444.486 766.648 344.286 766.648 C 244.085 766.648 162.857 685.419 162.857 585.219 C 162.857 485.019 244.085 403.791 344.286 403.791 C 444.486 403.791 525.714 485.019 525.714 585.219 Z"]
               RenderText {#text} at (0,0) size 0x0
           RenderTableRow {TR} at (0,142) size 462x40
             RenderTableCell {TH} at (67,153) size 112x17 [bgcolor=#DDDD99] [r=4 c=1 rs=1 cs=1]

....

[Nikolas Zimmermann]

Just a note, Zoltans getCTM patch is included in my recent patch at bug 47467, revisiting this topic again.

[Csaba Osztrogonác]

One more failing test: svg/dom/css-transforms.xhtml
See https://bugs.webkit.org/show_bug.cgi?id=71309#c25 for details.

[Nikolas Zimmermann]

(In reply to comment #26)
> One more failing test: svg/dom/css-transforms.xhtml
> See https://bugs.webkit.org/show_bug.cgi?id=71309#c25 for details.
This should be fixed by r101342, we need to unskip it and try.

[Nikolas Zimmermann]

Created attachment 116947 [details]
Patch

Update Zoltans patch to ToT, and mac result changes, all of them are progressions, we obviously lost accurancy before.

[Zoltan Herczeg]

Switched the r? to me in the latest patch.

[Nikolas Zimmermann]

Comment on attachment 116947 [details]
Patch

Good patch, I'm landing it togeter with my mac baseline update.

[Nikolas Zimmermann]

Landed Zoltans patch in r101360, together with my mac results.
CC'ing Martin/Phil, as this might affect the new Gtk SVG baseline as well.

Leaving this bug open until Qt baseline is updated as well.

[Eric Seidel (no email)]

Attachment 116947 [details] was posted by a committer and has review+, assigning to Nikolas Zimmermann for commit.

[Eric Seidel (no email)]

Comment on attachment 116947 [details]
Patch

Cleared Nikolas Zimmermann's review+ from obsolete attachment 116947 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Csaba Osztrogonác]

Created attachment 134069 [details]
proposed patch

I think we should use similar fix to GTK and Chromium guys:
http://trac.webkit.org/changeset/103040/trunk/Tools/Scripts/webkitdirs.pm

I don't think if we should support older architecture than P4.

After this patch and rebasing now skipped tests related to this bug,
we can unskip ~70 tests, only 5 tests will remain with rounding issues.

[Simon Hausmann]

Comment on attachment 134069 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134069&action=review

The only thing I don't like about this is that we make a choice for everyone using WebKit on x86, just to "smoothen" layout test results. Think of people who'd like to pass -march=atom for example, or -msse3.

I know it causes other sorts of pain, but I wish we had a mode for this, something that

    (a) can be trivially enabled by WebKit developers
    (b) is checked against by (N)RWT.

Maybe this "feature" should be given a name and whether we're compiling with these flags or not should be recorded in .webkit.config. Then (N)RWT could check if it's enabled and if it's _not_ enabled it could either bail out or print a big error message.

Then we can use the compiler defaults and people who'd like to run the layout tests (on i386 only, btw) will learn about it and can enable the feature.

I'd love to also hear Tor Arne's opinion on this. We've had numerous discussions about things like developer modes and I understand that it's bad if we develop & test with something that's different than what's being shipped.

Either way, great job guys for drilling down cause for the layout test differences down to this :)

> Tools/qmake/mkspecs/features/unix/default_post.prf:8
> +# 387 to make layout test result same on 32 and on 64 bit builds.

I think it would be good here to have an explicit link to this bug. This is an important change.

> Tools/qmake/mkspecs/features/unix/default_post.prf:9
> +linux-g++*:isEqual(QT_ARCH,i386): QMAKE_CXXFLAGS += -march=pentium4 -msse2 -mfpmath=sse

QMAKE_CFLAGS should also be modified.

[Simon Hausmann]

Comment on attachment 134069 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134069&action=review

>> Tools/qmake/mkspecs/features/unix/default_post.prf:8
>> +# 387 to make layout test result same on 32 and on 64 bit builds.
> 
> I think it would be good here to have an explicit link to this bug. This is an important change.

As a side note: In chromium's common.gypi there's a _huuge_ comment about this.

[Martin Robinson]

(In reply to comment #35)
> (From update of attachment 134069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134069&action=review
> 
> The only thing I don't like about this is that we make a choice for everyone using WebKit on x86, just to "smoothen" layout test results. Think of people who'd like to pass -march=atom for example, or -msse3.

For what it's worth, for the GTK+ port we only do this in build-webkit which isn't used by our downstream consumers.

[Simon Hausmann]

Comment on attachment 134069 [details]
proposed patch

Ok, let's do this :). My earlier comments still apply, which means before landing I think the CFLAGS should be fixed and the comment should either be expanded or maybe pointed to this bug.

If it really turns out to be an issue for i386 packaging, then we know where to look.

[Csaba Osztrogonác]

(In reply to comment #38)
> (From update of attachment 134069 [details])
> Ok, let's do this :). My earlier comments still apply, which means before landing I think the CFLAGS should be fixed and the comment should either be expanded or maybe pointed to this bug.
> 
> If it really turns out to be an issue for i386 packaging, then we know where to look.

Thanks, will fix.

I found the chromium's comment about it:
http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi?view=markup

"All floating-point computations on x87 happens in 80-bit
precision.  Because the C and C++ language standards allow
the compiler to keep the floating-point values in higher
precision than what's specified in the source and doing so
is more efficient than constantly rounding up to 64-bit or
32-bit precision as specified in the source, the compiler,
especially in the optimized mode, tries very hard to keep
values in x87 floating-point stack (in 80-bit precision)
as long as possible. This has important side effects, that
the real value used in computation may change depending on
how the compiler did the optimization - that is, the value
kept in 80-bit is different than the value rounded down to
64-bit or 32-bit. There are possible compiler options to
make this behavior consistent (e.g. -ffloat-store would keep
all floating-values in the memory, thus force them to be
rounded to its original precision) but they have significant
runtime performance penalty.

-mfpmath=sse -msse2 makes the compiler use SSE instructions
which keep floating-point values in SSE registers in its
native precision (32-bit for single precision, and 64-bit
for double precision values). This means the floating-point
value used during computation does not change depending on
how the compiler optimized the code, since the value is
always kept in its specified precision."

[Csaba Osztrogonác]

Comment on attachment 134069 [details]
proposed patch

Landed in http://trac.webkit.org/changeset/112397

Test unskipping and rebaselining will be landed in a separated patch.

[Csaba Osztrogonác]

(In reply to comment #40)
> (From update of attachment 134069 [details])
> Landed in http://trac.webkit.org/changeset/112397
> 
> Test unskipping and rebaselining will be landed in a separated patch.

Rebaselines and unskipping landed in http://trac.webkit.org/changeset/112514

[Allan Sandfeld Jensen]

I think this is a problem. This means we require a processor with SSE2 for QtWebKit. Does that match the minimum requirements for Qt?

[Csaba Osztrogonác]

(In reply to comment #42)
> I think this is a problem. This means we require a processor with SSE2 for QtWebKit. Does that match the minimum requirements for Qt?

There wasn't any objection in the last 10 months ... 

I don't think if Pentium 4 is a big requirment nowadays.  Of course the build
is still possible without SSE2, but x87 is a little bit slower, not IEEE754
compatible, and ~20-30 layout test result are different a little bit on 32 bit.

[Simon Hausmann]

(In reply to comment #43)
> (In reply to comment #42)
> > I think this is a problem. This means we require a processor with SSE2 for QtWebKit. Does that match the minimum requirements for Qt?
> 
> There wasn't any objection in the last 10 months ... 
> 
> I don't think if Pentium 4 is a big requirment nowadays.  Of course the build
> is still possible without SSE2, but x87 is a little bit slower, not IEEE754
> compatible, and ~20-30 layout test result are different a little bit on 32 bit.

I agree with Ossy. This is about the fact that we require SSE2 if you'd like to run the _tests_ with QtWebKit. That means essentially a development environment requiring a at least SSE2 capable CPU. Not much of a burden I'd say.

[Allan Sandfeld Jensen]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > I think this is a problem. This means we require a processor with SSE2 for QtWebKit. Does that match the minimum requirements for Qt?
> > 
> > There wasn't any objection in the last 10 months ... 
> > 
> > I don't think if Pentium 4 is a big requirment nowadays.  Of course the build
> > is still possible without SSE2, but x87 is a little bit slower, not IEEE754
> > compatible, and ~20-30 layout test result are different a little bit on 32 bit.
> 
> I agree with Ossy. This is about the fact that we require SSE2 if you'd like to run the _tests_ with QtWebKit. That means essentially a development environment requiring a at least SSE2 capable CPU. Not much of a burden I'd say.

No, the problem is we enforce the use of SSE. We don't just enable them for people who wants to run the tests, but always. Perhaps there it should only be set for buildbot or developer builds?

[Allan Sandfeld Jensen]

> (In reply to comment #42)
> 
> Of course the build is still possible without SSE2,

How? Only by editing the build files as far as I can tell. I had to add an extra option to disable it in QtWebKit 2.3, since it would otherwise break minimum requirements of allmost all linux distros.

[Simon Hausmann]

(In reply to comment #46)
> > (In reply to comment #42)
> > 
> > Of course the build is still possible without SSE2,
> 
> How? Only by editing the build files as far as I can tell. I had to add an extra option to disable it in QtWebKit 2.3, since it would otherwise break minimum requirements of allmost all linux distros.

Right. We could add a !production_build qmake scope around there.