Last Comment Bug 680277 - Debug builds should always be compiled with frame pointers
: Debug builds should always be compiled with frame pointers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 669953 697572
  Show dependency treegraph
 
Reported: 2011-08-18 15:38 PDT by :Ehsan Akhgari
Modified: 2011-10-26 14:33 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.61 KB, patch)
2011-08-18 15:40 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (7.18 KB, patch)
2011-08-18 16:08 PDT, :Ehsan Akhgari
khuey: review+
Details | Diff | Splinter Review
Patch (v2) (11.26 KB, patch)
2011-08-22 13:47 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
centralize the logic for frame pointers and enable them on debug builds (16.92 KB, patch)
2011-08-30 10:33 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review-
Details | Diff | Splinter Review
updated patch (17.03 KB, patch)
2011-08-31 13:53 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
update clang (18.21 KB, patch)
2011-09-01 09:03 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
Fix another force of -fomit-frame-pointer (690 bytes, patch)
2011-09-01 13:25 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-08-18 15:38:57 PDT
Otherwise, the stackwalker code would break.  This was the problem behind bug 669953 comment 16 for example.
Comment 1 :Ehsan Akhgari 2011-08-18 15:40:00 PDT
Created attachment 554232 [details] [diff] [review]
Patch (v1)
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-18 15:57:12 PDT
Are there equivalents in js/src that should be changed as well?
Comment 3 :Ehsan Akhgari 2011-08-18 16:06:33 PDT
(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.
Comment 4 :Ehsan Akhgari 2011-08-18 16:08:47 PDT
Created attachment 554245 [details] [diff] [review]
Patch (v2)
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-18 16:09:44 PDT
(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!
Comment 6 Marco Bonardo [::mak] 2011-08-19 03:18:38 PDT
http://hg.mozilla.org/mozilla-central/rev/a8199bd4af83
Comment 7 :Ehsan Akhgari 2011-08-22 09:10:32 PDT
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.
Comment 8 :Ehsan Akhgari 2011-08-22 13:47:26 PDT
Created attachment 554950 [details] [diff] [review]
Patch (v2)

Second try...
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-23 16:03:08 PDT
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 Luke Wagner [:luke] 2011-08-25 11:45:55 PDT
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
Comment 11 :Ehsan Akhgari 2011-08-26 07:59:17 PDT
(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 Luke Wagner [:luke] 2011-08-26 08:18:48 PDT
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).
Comment 13 Gregory Szorc [:gps] 2011-08-26 11:59:36 PDT
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 Luke Wagner [:luke] 2011-08-26 18:08:43 PDT
(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 16 :Ehsan Akhgari 2011-08-27 15:49:25 PDT
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?
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-27 16:40:20 PDT
(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.
Comment 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 10:33:37 PDT
Created attachment 556906 [details] [diff] [review]
centralize the logic for frame pointers and enable them on debug builds
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 10:38:44 PDT
Try at http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=30b60a957d8d
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-31 12:45:33 PDT
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.
Comment 21 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-31 13:53:30 PDT
Created attachment 557308 [details] [diff] [review]
updated patch

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=75f96aa15f83
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-31 14:10:30 PDT
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
Comment 23 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-01 09:03:52 PDT
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.
Comment 24 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-01 09:06:47 PDT
Try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e8ca4b830dbd
Comment 25 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-01 13:25:22 PDT
Created attachment 557624 [details] [diff] [review]
Fix another force of -fomit-frame-pointer

I missed this one when I grepped for MODULE_OPTIMEZE_FLAGS :-(
Comment 27 Ed Morley [:emorley] 2011-09-02 01:58:45 PDT
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
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-02 04:09:39 PDT
Comment on attachment 557624 [details] [diff] [review]
Fix another force of -fomit-frame-pointer

I gave r+ over irc ... just doing some bookkeeping.
Comment 29 Ed Morley [:emorley] 2011-09-04 16:49:33 PDT
http://hg.mozilla.org/mozilla-central/rev/d1d3ad947e9f

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