Last Comment Bug 680515 - Setting MOZ_OPTIMIZE_FLAGS in js/src/configure.in is not effective
: Setting MOZ_OPTIMIZE_FLAGS in js/src/configure.in is not effective
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
: 464328 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-19 11:53 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-08-31 02:00 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch I am testing (948 bytes, patch)
2011-08-19 12:09 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
new patch (2.68 KB, patch)
2011-08-20 06:45 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
Add strict aliasing back (838 bytes, patch)
2011-08-20 17:19 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Add strict aliasing back, in pgo builds too :-( (884 bytes, patch)
2011-08-20 20:27 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch (5.26 KB, patch)
2011-08-22 08:21 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
ted: review+
Details | Diff | Splinter Review
enable strict aliasing in js/src (539 bytes, patch)
2011-08-30 05:21 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
really remove -fno-strict-aliasing from js/src (7.62 KB, patch)
2011-08-30 05:39 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review-
Details | Diff | Splinter Review
now in plain text :-( (1.80 KB, patch)
2011-08-30 05:40 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
enable strict aliasing and add -fno-stack-protector (2.69 KB, patch)
2011-08-30 05:42 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-19 11:53:19 PDT
When MODULE_OPTIMIZE_FLAGS is defined, it is used instead of MOZ_OPTIMIZE_FLAGS. There many places in js/src/configure.in that set MOZ_OPTIMIZE_FLAGS. Those are probably bugs.

khuey suggesting just dropping MODULE_OPTIMIZE_FLAGS from js/src, I am testing such a patch.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-19 12:09:56 PDT
Created attachment 554502 [details] [diff] [review]
patch I am testing

This fixes the particular problem we were having (-Qy not being used when building js). The try is at

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d1b64105ac11
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-19 13:12:51 PDT
Comment on attachment 554502 [details] [diff] [review]
patch I am testing

I'm ok with ripping this stuff out ... if we break something it's going to be icc or Solaris and those people can move the logic into configure.
Comment 3 :Ehsan Akhgari 2011-08-19 15:16:39 PDT
Landed on inbound.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-08-19 16:28:50 PDT
*** Bug 464328 has been marked as a duplicate of this bug. ***
Comment 5 Phil Ringnalda (:philor) 2011-08-19 18:45:18 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> I'm ok with ripping this stuff out ... if we break something it's going to
> be icc or Solaris and those people can move the logic into configure.

Nope, it's going to be OS X, and those people can back you out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8f1ad97f4bc2.

10.5 opt, pretty much instant crash in every test suite. 10.6, tests were fine, but 62 jit_test failures.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-19 18:50:43 PDT
Looks like we lost -fstrict-aliasing -fno-stack-protector
Comment 7 Luke Wagner [:luke] 2011-08-19 18:57:15 PDT
In case you didn't already see, the -fno-stack-protector prevents some pretty lame reappearing compiler bugs that cause js::Interpret to crash (see bug 644250).
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-19 20:43:17 PDT
Sorry for the problems. New try:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=f408343c8225
Comment 9 Bill McCloskey (:billm) 2011-08-19 20:48:39 PDT
As long as this is going to be MacOS-only, could we take out -fstrict-aliasing? Only -fno-stack-protector is needed to eliminate the crashes.
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 06:43:14 PDT
We could, but why?
Having strict aliasing is probably a good thing.
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 06:45:05 PDT
Created attachment 554632 [details] [diff] [review]
new patch
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-20 06:49:18 PDT
Comment on attachment 554632 [details] [diff] [review]
new patch

If we want to drop -fstrict-aliasing, lets do that in a separate bug.
Comment 13 Mike Hommey [:glandium] 2011-08-20 06:52:41 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> If we want to drop -fstrict-aliasing, lets do that in a separate bug.

That's bug 414641
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-20 06:57:33 PDT
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> > If we want to drop -fstrict-aliasing, lets do that in a separate bug.
> 
> That's bug 414641

Well that's to turn it on for the browser.  Bill is talking about turning it off for js.
Comment 15 Bill McCloskey (:billm) 2011-08-20 08:37:10 PDT
As far as I can tell, this patch turns it off on every platform but MacOS. That just doesn't make sense to me. I think we should either enable it on all platforms or disable it on all platforms.

Also, bug 657806 is about disabling it for the JS engine.
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 15:22:07 PDT
As far as I can tell, linux and android already set -fomit-frame-pointer in the configure.
Comment 17 Bill McCloskey (:billm) 2011-08-20 15:51:47 PDT
Sorry, I was talking about -fstrict-aliasing. In fact, the configure script quite explicitly uses -fno-strict-aliasing on all GNU platforms. The only reason we got strict aliasing before was because MOZ_OPTIMIZE_FLAGS appears last. So the command line looked like:
  g++ ... -fno-strict-aliasing ... -fstrict-aliasing ... file.cpp
           ^ from configure         ^ from MOZ_OPTIMIZE_FLAGS
The second option was overriding the first.
Comment 18 Bill McCloskey (:billm) 2011-08-20 15:54:49 PDT
Oops, in comment 17 I meant MODULE_OPTIMIZE_FLAGS, not MOZ_OPTIMIZE_FLAGS.
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 17:19:10 PDT
Created attachment 554681 [details] [diff] [review]
Add strict aliasing back

You are right, the attached patch adds -fstrict-aliasing back.

I did the change in the Makefile since I am investigating how we could integrate configure and js/src/configure, but I can change configure if you prefer.

Try at:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=b1dd998cb18c
Comment 20 Bill McCloskey (:billm) 2011-08-20 19:05:18 PDT
Comment on attachment 554681 [details] [diff] [review]
Add strict aliasing back

I'm not a good person to review this. I don't know much about our build system.
Comment 21 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 20:27:53 PDT
Created attachment 554691 [details] [diff] [review]
Add strict aliasing back, in pgo builds too :-(
Comment 22 Makoto Kato [:m_kato] 2011-08-21 01:00:36 PDT
MSVC target, optimize flag seems to be modified from -O2 to -O1 by this fix.  Should you change MOZ_OPTIMIZE_FLAGS for *-ming* to -O2 instead of -O1 in js/src/configure.in?
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2011-08-21 11:30:13 PDT
http://hg.mozilla.org/mozilla-central/rev/53f198cfbf47
Comment 24 Matt Brubeck (:mbrubeck) 2011-08-22 08:00:31 PDT
This changes the optimization flags for JS, and regressed performance on Windows and Android.
Comment 25 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-22 08:21:45 PDT
Created attachment 554869 [details] [diff] [review]
Patch

I had to back this out again because it caused perf regressions on Android.

http://hg.mozilla.org/mozilla-central/rev/3d32e28b81a0
http://hg.mozilla.org/mozilla-central/rev/c9f050fe21a3

I went through and transplanted the flags from the Makefile to configure, and dropped -fstrict-aliasing.  I think I preserved everything (except the icc flags), but would like a second pair of eyes.

Sorry about all this Rafael, I should have exercised more diligence when reviewing this :-/
Comment 26 Ted Mielczarek [:ted.mielczarek] 2011-08-25 13:15:03 PDT
Comment on attachment 554869 [details] [diff] [review]
Patch

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

Thanks for the cleanup, I've wanted this to get done forever.

::: js/src/configure.in
@@ +2163,2 @@
>      fi
> +    MOZ_OPTIMIZE_FLAGS="-O3 -freorder-blocks -fno-reorder-functions $MOZ_FRAMEPTR_FLAGS"

Do we really want -O3 on Android? (I guess that's what we've been shipping with, but do we *really* want it?)
Comment 27 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 13:19:56 PDT
(In reply to Ted Mielczarek [:ted, :luser] from comment #26)
> Do we really want -O3 on Android? (I guess that's what we've been shipping
> with, but do we *really* want it?)

I don't know.  I'll file a followup to investigate.
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-28 06:01:27 PDT
http://hg.mozilla.org/mozilla-central/rev/00d1dbeed5e4
http://hg.mozilla.org/mozilla-central/rev/671001912407

Will be watching carefully for Talos regressions.
Comment 29 Ed Morley [:emorley] 2011-08-30 04:24:00 PDT
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/k-SVzslmT5s
{
Dromaeo (V8) decrease 4.31% on Linux x64 Mozilla-Inbound
Previous: avg 172.679 stddev 1.739 of 30 runs up to revision a12d726a5868
    New     : avg 165.239 stddev 1.545 of 5 runs since revision ca5a3569462d
    Change  : -7.440 (4.31% / z=4.279)
}


https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/KRVrqBg_qPs
{
Dromaeo (V8) decrease 4.39% on Linux x64 Firefox
    Previous: avg 174.054 stddev 1.437 of 30 runs up to revision 6c8a909977d3
    New     : avg 166.406 stddev 0.525 of 5 runs since revision 0ac24f429e24
    Change  : -7.648 (4.39% / z=5.322)

Dromaeo (String/Array/Eval/Regex) decrease 3.69% on Linux Firefox
    Previous: avg 693.361 stddev 5.866 of 30 runs up to revision 6c8a909977d3
    New     : avg 667.757 stddev 3.319 of 5 runs since revision 0ac24f429e24
    Change  : -25.604 (3.69% / z=4.365)
}
Comment 30 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 04:35:52 PDT
The thing I don't understand about the Talos regressions is that if they were due to this I'd expect them to be across the board, not in a few isolated tests.
Comment 31 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 05:06:57 PDT
This would be because of disabling strict aliasing in http://hg.mozilla.org/mozilla-central/rev/00d1dbeed5e4 I assume? I would suggest enabling it back to see if it is fixed.
Comment 32 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 05:09:50 PDT
That's plausible.  I'll give that a shot.
Comment 33 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 05:21:06 PDT
Created attachment 556803 [details] [diff] [review]
enable strict aliasing in js/src

http://hg.mozilla.org/try/rev/968b1ff08ce3
Comment 34 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 05:21:21 PDT
Also could be due to dropping -fno-stack-protector.  Unlike the strict aliasing change, that was linux only.
Comment 35 Mike Hommey [:glandium] 2011-08-30 05:26:38 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #34)
> Also could be due to dropping -fno-stack-protector.  Unlike the strict
> aliasing change, that was linux only.

Unlikely, that one has no meaning unless you do enable -fstack-protector. And IIRC it was added because some distro default to that and that was breaking the js engine. Which means removing it is BAD™.
Comment 36 Mike Hommey [:glandium] 2011-08-30 05:29:51 PDT
Looks like I'm partially wrong.

changeset:   64354:663a3afb238e
user:        Luke Wagner <lw@mozilla.com>
date:        Wed Mar 23 00:39:59 2011 -0700
summary:     Mac crashes from cf1850399b0b seem to be in bogo regalloc for -fstack-protector stack guard.  Try turning it off temporarily to see if that fixes the problem
Comment 37 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 05:39:13 PDT
Created attachment 556807 [details] [diff] [review]
really remove -fno-strict-aliasing from js/src

Try (with talos) is running at:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=44d91488601a
Comment 38 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 05:40:10 PDT
Comment on attachment 556807 [details] [diff] [review]
really remove -fno-strict-aliasing from js/src

Don't think you meant to upload this ...
Comment 39 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 05:40:55 PDT
Created attachment 556808 [details] [diff] [review]
now in plain text :-(

Sorry about the noise, the previous patch was in html.
Comment 40 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 05:42:54 PDT
Created attachment 556809 [details] [diff] [review]
enable strict aliasing and add -fno-stack-protector

Try running at http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732

Let me know which of these patches you like best.
Comment 41 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-30 05:43:44 PDT
Lets skip the stack protector stuff for now and just do aliasing.
Comment 42 Luke Wagner [:luke] 2011-08-30 07:54:07 PDT
(In reply to Mike Hommey [:glandium] from comment #35)
> And IIRC it was added because some distro default to that and that was
> breaking the js engine. Which means removing it is BAD™.

'twas OS X.  bug 644250.
Comment 43 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-30 08:28:32 PDT
Are talus runs on Try and Inbound comparable? I got 346 in

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732
Comment 44 Brian Hackett (:bhackett) 2011-08-30 08:38:17 PDT
This might depend on whether you are comparing runs on either side of the TI merge, which I think doubled the dromaeo_v8 score.
Comment 45 Marco Bonardo [::mak] 2011-08-31 02:00:07 PDT
http://hg.mozilla.org/mozilla-central/rev/22cc14edadaa

please reopen bugs or create followup bugs, pushing patches on resolved fixed bugs is evil :)

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