Last Comment Bug 413019 - crashes with WPO (-GL) in cairo
: crashes with WPO (-GL) in cairo
Status: VERIFIED FIXED
[ccbr][inbound]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla7
Assigned To: Steve Snyder
:
Mentors:
Depends on:
Blocks: 411369
  Show dependency treegraph
 
Reported: 2008-01-18 14:21 PST by Robert Sayre
Modified: 2011-08-29 02:22 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial removal of PGO breakage work-around (964 bytes, patch)
2009-07-21 17:50 PDT, Steve Snyder
ted: review+
Details | Diff | Review
Further reduction in scope of work-around (1.99 KB, patch)
2010-05-11 04:23 PDT, Steve Snyder
no flags Details | Diff | Review
fix (2.26 KB, patch)
2011-06-29 20:49 PDT, Makoto Kato [:m_kato]
ted: review+
Details | Diff | Review

Description Robert Sayre 2008-01-18 14:21:34 PST
Bug 411639 turns this off for image decoders, pixman, xptcall, and cairo
Comment 1 Robert Sayre 2008-01-18 14:44:52 PST
Correction, that's bug 411369. I'm filing this so I have something to point FIXME comments at.
Comment 2 Steve Snyder 2009-07-18 15:14:01 PDT
Now that libpng no longer includes any assembly code, let alone MMX, can the -GL- work-around be removed from the build?

~/modules/libimg/png/Makefile.in:

ifeq ($(OS_ARCH),WINNT)
# FIXME: bug 413019
OS_COMPILE_CFLAGS += -GL-
endif
Comment 3 Steve Snyder 2009-07-18 15:28:06 PDT
Also, from looking at the cairo/libpixman code in Gecko v1.9.1.1, it seems that there's no longer any MMX in assembly there either.  The remaining ASM code is either GCC-specific or just used to get the x86 feature flags.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2009-07-21 17:30:59 PDT
We could certainly try it, write a patch?
Comment 5 Steve Snyder 2009-07-21 17:50:27 PDT
Created attachment 389828 [details] [diff] [review]
Partial removal of PGO breakage work-around

This patch removes the -GL- work-around in the Cairo and PNG libraries.  I saw no adverse reactions to this in a PGO build of Firefox v3.5.1 (on a Pentium4-based Win2K/SP4 system).

The work-around is left intact in the libpixman library because removal caused crashes on Firefix start-up.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2009-07-22 10:57:15 PDT
Comment on attachment 389828 [details] [diff] [review]
Partial removal of PGO breakage work-around

If you could run your PGO build through the unittests I'd feel even better about this, but it's definitely worth a shot.
Comment 7 Daniel Veditz [:dveditz] 2009-11-11 16:23:57 PST
Is this fix ready to check in?
Comment 8 Ted Mielczarek [:ted.mielczarek] 2009-11-11 16:30:58 PST
I'd say it could stand a pass by the try server, but I'm pretty sure that doesn't do PGO builds. We have unittests on PGO builds on mozilla-central, so I think it should be pretty safe to checkin there. If it causes any bustage we can back it out.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2010-03-23 12:59:02 PDT
Dietrich, possible perf win here, but needs testing. Maybe via the Places branch?
Comment 10 Steve Snyder 2010-05-11 04:23:41 PDT
Created attachment 444630 [details] [diff] [review]
Further reduction in scope of work-around

Restored PGO optimization to all libpixman sources files except pixman-mmx.c and pixman-sse2.c.  Only these 2 files are adversely affected by VS2005/SP1 PGO.  This patch applies cleanly to Firefox v3.6.4 Release Candidate Build #3

Tested under WinXP/SP3 on Pentium3 and Core2 Duo CPUs (to exercise the MMX and SSE2 code paths).  As expected, the complete removal of the PGO disabling causes the browser to crash on start-up.  The crashing is cured on on a given system by disabling PGO for the SIMD code path that that system uses at runtime. The other 24 source files can safely be optimized.

It is possible that the disabling of PGO is also required for source file pixman-vmx.c, but I can't test that.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2011-01-21 05:42:19 PST
In the time since comment 8 was made, the try server now does PGO builds, so a spin past the try server should be sufficient for testing.
Comment 12 Makoto Kato [:m_kato] 2011-06-29 20:49:21 PDT
Created attachment 543049 [details] [diff] [review]
fix
Comment 13 Makoto Kato [:m_kato] 2011-06-29 20:49:40 PDT
passed on try server
http://tbpl.mozilla.org/?tree=Try&rev=b60bd9b73fd0
Comment 14 Makoto Kato [:m_kato] 2011-06-29 20:56:32 PDT
Comment on attachment 543049 [details] [diff] [review]
fix

Now this is passed on try server.

Should I retry this after changed to VS2010 (after next merge cycle)?

Also, I remove PGO workaround into jpeg.  Because now, we switch to libjepg-turbo.  (Original libjpeg + our SSE2 code had inline assembler.)
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-06-30 11:37:22 PDT
Comment on attachment 543049 [details] [diff] [review]
fix

Review of attachment 543049 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine. I wouldn't worry about re-testing with Visual C++ 2010, we should be able to see if our crash-stats change when we switch.
Comment 17 Marco Bonardo [::mak] 2011-07-02 02:37:00 PDT
http://hg.mozilla.org/mozilla-central/rev/26ecb4d6c3d0
Comment 18 Simona B [:simonab](PTO) 2011-08-29 02:22:46 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

I visually inspected the code changes. Marking as Verified Fixed as per bug 659997 Comment 7.

Note You need to log in before you can comment on or make changes to this bug.