Debug builds should always be compiled with frame pointers

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: espindola)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Otherwise, the stackwalker code would break.  This was the problem behind bug 669953 comment 16 for example.
Created attachment 554232 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #554232 - Flags: review?(khuey)
Are there equivalents in js/src that should be changed as well?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Are there equivalents in js/src that should be changed as well?

Yes, sorry I forgot that.  New patch coming up.
Created attachment 554245 [details] [diff] [review]
Patch (v2)
Attachment #554232 - Attachment is obsolete: true
Attachment #554232 - Flags: review?(khuey)
Attachment #554245 - Flags: review?(khuey)
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> > Are there equivalents in js/src that should be changed as well?
> 
> Yes, sorry I forgot that.  New patch coming up.

No need to apologize, that's what review is for!
Attachment #554245 - Flags: review?(khuey) → review+
http://hg.mozilla.org/mozilla-central/rev/a8199bd4af83
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
This fix is not good enough, because things like --enable-debug and --enable-debug-symbols override MOZ_DEBUG_FLAGS.  I'm gonna write a new patch here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 554950 [details] [diff] [review]
Patch (v2)

Second try...
Attachment #554950 - Flags: review?(khuey)
If frame pointers are the only thing we want to do this for, lets make MOZ_DEBUG_FLAGS_EXTRA MOZ_FRAMEPTR_FLAGS and rebase this on top of Bug 680515.

Comment 10

6 years ago
I posted a few days ago to dev-tree-management that it seems like this cset was responsible for multiple Dromaeo and other Talos regressions:
http://groups.google.com/group/mozilla.dev.tree-management/msg/ab186ed49729d03a
(In reply to Luke Wagner [:luke] from comment #10)
> I posted a few days ago to dev-tree-management that it seems like this cset
> was responsible for multiple Dromaeo and other Talos regressions:
> http://groups.google.com/group/mozilla.dev.tree-management/msg/
> ab186ed49729d03a

I find this hard to believe...  I explicitly verified that the compiler options being passed in both build logs are the same, and besides that, there are no code changes involved here.

Comment 12

6 years ago
Well, I don't know what to tell you; did you verify this on Windows (WINNT6.1 and 5.1)?

To be clear, this is a 6% Dromaeo(CSS) and 7% Dromaeo(SunSpider) regression (http://graphs-new.mozilla.org/graph.html#tests=[[75,1,12],[72,1,12]]&sel=1313671143051.916,1313846242335.437&displayrange=30&datatype=running) which has yet to be recovered.  The regression was pretty clearly bisected to a8199bd4af83 by try-talos (see the links in the dev.tree-management post).
It makes sense to me that there was a performance regression.

The frame pointer occupies a register. When you compile without the frame pointer, that gives the compiler an extra register to utilize (for function arguments, variables, etc). When you take this register away, which we have done in this bug, the compiler will have to put things on the stack, which is backed by physical memory, which is much, much slower than a register.

Comment 14

6 years ago
(In reply to Gregory Szorc [:gps] from comment #13)
In theory, the patch only turns of the frame-pointer optimization for debug builds.  My guess is that the bug is somehow its happening for release builds.

Comment 15

6 years ago
Ah, I see there was a backout:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d369283b0c70
and indeed the regression went away:
http://graphs-new.mozilla.org/graph.html#tests=[[72,63,12],[75,63,12]]&sel=1314333974448.347,1314407639955&displayrange=7&datatype=running
I still can't explain why this regression happened, but yes, the backout indeed has caused it to go away.  The weird thing is that as far as I can see from the build logs, -Oy- is not passed to the compiler.  Kyle, do you have any idea on why this regression could have happened?
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> I still can't explain why this regression happened, but yes, the backout
> indeed has caused it to go away.  The weird thing is that as far as I can
> see from the build logs, -Oy- is not passed to the compiler.  Kyle, do you
> have any idea on why this regression could have happened?

So, poking around a bit, I think that MOZ_DEBUG_FLAGS is passed to the compiler if --enable-debug-symbols is used, which is the default.  This likely caused us to start passing -Oy- to the compiler.
Assignee: ehsan → respindola
Created attachment 556906 [details] [diff] [review]
centralize the logic for frame pointers and enable them on debug builds
Attachment #556906 - Flags: review?(khuey)
Try at http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=30b60a957d8d
Attachment #554950 - Attachment is obsolete: true
Attachment #554950 - Flags: review?(khuey)
Attachment #554245 - Attachment is obsolete: true
Comment on attachment 556906 [details] [diff] [review]
centralize the logic for frame pointers and enable them on debug builds

>diff --git a/aclocal.m4 b/aclocal.m4
>--- a/aclocal.m4
>+++ b/aclocal.m4
>@@ -11,16 +11,17 @@ builtin(include, build/autoconf/pkg.m4)d
> builtin(include, build/autoconf/freetype2.m4)dnl
> builtin(include, build/autoconf/codeset.m4)dnl
> builtin(include, build/autoconf/altoptions.m4)dnl
> builtin(include, build/autoconf/mozprog.m4)dnl
> builtin(include, build/autoconf/mozheader.m4)dnl
> builtin(include, build/autoconf/acwinpaths.m4)dnl
> builtin(include, build/autoconf/lto.m4)dnl
> builtin(include, build/autoconf/gcc-pr49911.m4)dnl
>+builtin(include, build/autoconf/frameptr.m4)dnl

I'm starting to like moving crap out of configure.

>diff --git a/build/autoconf/frameptr.m4 b/build/autoconf/frameptr.m4
>new file mode 100644
>--- /dev/null
>+++ b/build/autoconf/frameptr.m4
>@@ -0,0 +1,21 @@
>+dnl Set MOZ_FRAMEPTR_FLAGS to the flags that should be used for enabling or
>+dnl disabling frame pointers in this architecture based on the configure
>+dnl options
>+
>+AC_DEFUN([MOZ_SET_FRAMEPTR_FLAGS], [
>+  if test "$GNU_CC"; then
>+    MOZ_ENABLE_FRAME_PTR="-fno-omit-frame-pointer"
>+    MOZ_DISABLE_FRAME_PTR="-fomit-frame-pointer"
>+  else
>+    MOZ_ENABLE_FRAME_PTR="-Oy-"
>+    MOZ_DISABLE_FRAME_PTR="-Oy"
>+  fi

This check is broken for at least Solaris, and likely other platforms (any Unix that doesn't use gcc, icc, and I would assume clang except that since you wrote it I guess GNU_CC is defined for clang?).  The r- is for that.  I think it would be ok to set the platforms we know explicitly, and then just warn and do nothing (use the compiler default, in other words) on other platforms.

>+  # if we are debugging or profiling, we want a frame pointer.

Complete nit, but can this comment reflect the sense of the test?  It took me a minute or two to parse this because the comment refers to the else block here ...

Other than that it looks good.
Attachment #556906 - Flags: review?(khuey) → review-
Created attachment 557308 [details] [diff] [review]
updated patch

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=75f96aa15f83
Attachment #556906 - Attachment is obsolete: true
Attachment #557308 - Flags: review?(khuey)
Comment on attachment 557308 [details] [diff] [review]
updated patch

I was a bit uneasy at first about comparing target to mingw here instead of going for something msvc specific, but we already do that.

r=me
Attachment #557308 - Flags: review?(khuey) → review+
Created attachment 557517 [details] [diff] [review]
update clang

The only difference is an update to the hack in xpcom/reflect/xptcall/src/md/unix/Makefile.in to use the new MOZ_FRAMEPTR_FLAGS variable.
Attachment #557308 - Attachment is obsolete: true
Attachment #557517 - Flags: review?(khuey)
Try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e8ca4b830dbd
Attachment #557517 - Flags: review?(khuey) → review+
Created attachment 557624 [details] [diff] [review]
Fix another force of -fomit-frame-pointer

I missed this one when I grepped for MODULE_OPTIMEZE_FLAGS :-(
Attachment #557624 - Flags: review?(khuey)
Comment on attachment 557517 [details] [diff] [review]
update clang

http://hg.mozilla.org/integration/mozilla-inbound/rev/11247af82311
http://hg.mozilla.org/integration/mozilla-inbound/rev/e3626f903f9f
All three recent changesets backed out (comment 26 and one more that was never posted here):
http://hg.mozilla.org/integration/mozilla-inbound/rev/deec82bfd178
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: mozilla9 → ---
Comment on attachment 557624 [details] [diff] [review]
Fix another force of -fomit-frame-pointer

I gave r+ over irc ... just doing some bookkeeping.
Attachment #557624 - Flags: review?(khuey) → review+
http://hg.mozilla.org/mozilla-central/rev/d1d3ad947e9f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

6 years ago
Blocks: 697572
You need to log in before you can comment on or make changes to this bug.