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)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla9
People
(Reporter: ehsan.akhgari, Assigned: espindola)
References
Details
Attachments
(2 files, 5 obsolete files)
18.21 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
690 bytes,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Otherwise, the stackwalker code would break. This was the problem behind bug 669953 comment 16 for example.
Reporter | ||
Comment 1•13 years ago
|
||
Are there equivalents in js/src that should be changed as well?
Reporter | ||
Comment 3•13 years ago
|
||
(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.
Reporter | ||
Comment 4•13 years ago
|
||
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+
Comment 6•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a8199bd4af83
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Reporter | ||
Comment 7•13 years ago
|
||
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 → ---
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•13 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
Reporter | ||
Comment 11•13 years ago
|
||
(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•13 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).
Comment 13•13 years ago
|
||
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•13 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•13 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
Reporter | ||
Comment 16•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: ehsan → respindola
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #556906 -
Flags: review?(khuey)
Assignee | ||
Comment 19•13 years ago
|
||
Try at http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=30b60a957d8d
Reporter | ||
Updated•13 years ago
|
Attachment #554950 -
Attachment is obsolete: true
Attachment #554950 -
Flags: review?(khuey)
Reporter | ||
Updated•13 years ago
|
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-
Assignee | ||
Comment 21•13 years ago
|
||
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+
Assignee | ||
Comment 23•13 years ago
|
||
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)
Assignee | ||
Comment 24•13 years ago
|
||
Try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e8ca4b830dbd
Attachment #557517 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 25•13 years ago
|
||
I missed this one when I grepped for MODULE_OPTIMEZE_FLAGS :-(
Attachment #557624 -
Flags: review?(khuey)
Comment 26•13 years ago
|
||
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
Comment 27•13 years ago
|
||
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+
Comment 29•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d1d3ad947e9f
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•