Closed Bug 680277 Opened 13 years ago Closed 13 years ago

Debug builds should always be compiled with frame pointers

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla9

People

(Reporter: ehsan.akhgari, Assigned: espindola)

References

Details

Attachments

(2 files, 5 obsolete files)

Otherwise, the stackwalker code would break.  This was the problem behind bug 669953 comment 16 for example.
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attached patch Patch (v2) (obsolete) — Splinter Review
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!
http://hg.mozilla.org/mozilla-central/rev/a8199bd4af83
Status: ASSIGNED → RESOLVED
Closed: 13 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 → ---
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
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.
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.
(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.
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
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-
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+
Attached patch update clangSplinter Review
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)
I missed this one when I grepped for MODULE_OPTIMEZE_FLAGS :-(
Attachment #557624 - Flags: review?(khuey)
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
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Blocks: 697572
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.