If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Pure JS code causes PGO breakages in js-ctypes

RESOLVED FIXED in mozilla19

Status

()

Core
js-ctypes
--
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: m_kato)

Tracking

unspecified
mozilla19
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Bug 794091 introduces two pure JavaScript patches that call into js-ctypes. These patches work nicely but break Win 64 PGO builds with the following error:

e:\builds\moz2_slave\try-w64\build\js\src\ctypes\ctypes.cpp(1687) : fatal error C1001: An internal error has occurred in the compiler.

Line 1687 points to function |jsvalToSize|.

This error is blocking landing of off main thread Session Store writes, which is itself one of the high priority Snappy goals.

Full TryServer log: https://tbpl.mozilla.org/php/getParsedLog.php?id=16935855&tree=Try#error2
(Assignee)

Comment 1

5 years ago
I will try some options...   Maybe, we should use mozjs.dll for Win64, too. Or, turn off PGO for ctypes only.
Assignee: nobody → m_kato
(Assignee)

Comment 2

5 years ago
Also, PGOCVT "internal errors" issue is filed as https://connect.microsoft.com/VisualStudio/feedback/details/766253 for VS2012.  PG0001 doesn't depend on bug 794091.  (Before bug 794091, this occurs...)
It seems that |#pragma optimize("", off)| around the breakage site works around the issue:
https://tbpl.mozilla.org/?tree=Try&rev=e04004655e71

I will clean up the patch and submit.
(Assignee)

Comment 4

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> It seems that |#pragma optimize("", off)| around the breakage site works
> around the issue:
> https://tbpl.mozilla.org/?tree=Try&rev=e04004655e71
> 
> I will clean up the patch and submit.

Can we use optimize("g", off) instead of?  "g" means -GL, and "" means turn off all optimization.  We should keep stack protection (/GS option) if possible
(In reply to Makoto Kato from comment #4)
> Can we use optimize("g", off) instead of?  "g" means -GL, and "" means turn
> off all optimization.  We should keep stack protection (/GS option) if
> possible

Checking out: https://tbpl.mozilla.org/?tree=Try&rev=ff8986e49b4a
So far, no success with "g". However, no success anymore with "" since I attempted to clean up my code. I will keep expanding |optimize("", off)| until it works. Makoko Kato, could you also test your approach in the meantime?
What might work is this:

CTypes.$(OBJ_SUFFIX): PROFILE_GEN_CFLAGS=

in js/src/Makefile.in (with the proper ifeq for win64)
(Assignee)

Comment 8

5 years ago
successful with -GL-.  Some cpp files uses this option for Win32 to turn on PGO.
https://tbpl.mozilla.org/?tree=Try&rev=c33c6862c06c
(Assignee)

Comment 9

5 years ago
Created attachment 680879 [details] [diff] [review]
fix
Attachment #680879 - Flags: review?(khuey)
(Assignee)

Comment 10

5 years ago
Comment on attachment 680879 [details] [diff] [review]
fix

># HG changeset patch
># Parent 5516d85af0928b6b7cdd250c1abe5879565db2f6
>Bug 810661 - Don't PGO for Ctypes.cpp
>
>diff --git a/js/src/Makefile.in b/js/src/Makefile.in
>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>@@ -716,16 +716,20 @@ CFLAGS += -fp:precise
> 
> ifeq ($(CPU_ARCH),x86)
> # Workaround compiler bug on PGO (Bug 721284)
> MonoIC.$(OBJ_SUFFIX): CXXFLAGS += -GL-
> Compiler.$(OBJ_SUFFIX): CXXFLAGS += -GL-
> # Ditto (Bug 772303)
> RegExp.$(OBJ_SUFFIX): CXXFLAGS += -GL-
> endif
>+# Ditto (Bug 810661)
>+ifeq ($(CPU_ARCH),x86_64)
>+CTypes.$(OBJ_SUFFIX): CXXFLAGS += -GL-
>+endif
> endif # _MSC_VER
> 
> ifeq ($(OS_ARCH),FreeBSD)
> EXTRA_LIBS	+= -pthread
> endif
> ifeq ($(OS_ARCH),Linux)
> EXTRA_LIBS	+= -ldl
> endif
Attachment #680879 - Attachment is patch: true
Comment on attachment 680879 [details] [diff] [review]
fix

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

Fun.
Attachment #680879 - Flags: review?(khuey) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5ad849146f
Target Milestone: --- → mozilla19
Created attachment 680927 [details] [diff] [review]
Alternative fix

Attaching an alternative fix suggested by glandium.
TryRun: https://tbpl.mozilla.org/?tree=Try&rev=6fb38a606e45

I am not versed enough in VC++ to determine which variant of the patch is best.
(Assignee)

Comment 14

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #13)
> Created attachment 680927 [details] [diff] [review]
> Alternative fix
> 
> Attaching an alternative fix suggested by glandium.
> TryRun: https://tbpl.mozilla.org/?tree=Try&rev=6fb38a606e45
> 
> I am not versed enough in VC++ to determine which variant of the patch is
> best.

Yoric, I already landed workaround in m-i.  glandium suggestion is same as changeset 8c5ad849146f.
(In reply to Makoto Kato from comment #14)
> Yoric, I already landed workaround in m-i.  glandium suggestion is same as
> changeset 8c5ad849146f.

Ah, great, thanks.
https://hg.mozilla.org/mozilla-central/rev/8c5ad849146f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.