Last Comment Bug 676607 - Patch our gcc 4.5 to fix gcc's PR49911
: Patch our gcc 4.5 to fix gcc's PR49911
Status: RESOLVED FIXED
[qa-]
:
Product: Release Engineering
Classification: Other
Component: Other (show other bugs)
: other
: x86_64 Linux
: P4 normal (vote)
: ---
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
http://gcc.gnu.org/bugzilla/show_bug....
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-04 11:36 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2013-08-12 21:54 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
proposed patch (9.64 KB, patch)
2011-08-04 11:36 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
New patch I am testing (9.94 KB, patch)
2011-08-10 15:45 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
catlee: review+
Details | Diff | Splinter Review
updated patch (10.64 KB, patch)
2011-08-17 14:36 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
deploy gcc test packages (2.91 KB, patch)
2011-08-18 09:08 PDT, Chris AtLee [:catlee]
bear: review+
catlee: checked‑in+
Details | Diff | Splinter Review
pr676607.patch (995 bytes, patch)
2011-08-21 07:57 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
new patch (10.34 KB, patch)
2011-08-22 06:42 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
bhearsum: review+
Details | Diff | Splinter Review
Alternative patch, use the already deployed compiler. (143.13 KB, patch)
2011-08-23 08:12 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
catlee: review-
Details | Diff | Splinter Review
install gcc-4.5-0moz2 package (1.71 KB, patch)
2011-09-07 06:00 PDT, Chris AtLee [:catlee]
bhearsum: review+
catlee: checked‑in+
Details | Diff | Splinter Review
build gcc-4.5-0moz2 into /tools/gcc-4.5-0moz2 (10.69 KB, patch)
2011-09-07 06:03 PDT, Chris AtLee [:catlee]
bhearsum: review+
catlee: checked‑in+
Details | Diff | Splinter Review
update the mozconfigs (10.35 KB, patch)
2011-11-22 14:24 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
catlee: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-04 11:36:26 PDT
Created attachment 550765 [details] [diff] [review]
proposed patch

With this patch I am able to creating a working firefox debug build with optimizations on 32 bit centos.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-10 15:45:37 PDT
Created attachment 552248 [details] [diff] [review]
New patch I am testing

As discussed on IRC, this patch changes the name and install dir so that it can be installed in parallel for testing.
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-10 16:36:02 PDT
The spec file builds and installs OK next to the existing gcc rpm.

Is the patch OK if firefox builds with both the old and new compilers finish OK?
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-11 09:09:57 PDT
The two builds are done, is the patch OK?
Comment 4 Chris AtLee [:catlee] 2011-08-17 11:14:04 PDT
gcc-4.5 fails to build on linux 64 with this error:

/bin/sh ../.././gcc/../move-if-change tmp-gi.list gtyp-input.list
echo timestamp > s-gtyp-input
build/gengtype ../.././gcc gtyp-input.list
make[3]: *** [s-gtype] Segmentation fault
make[3]: Leaving directory `/usr/src/redhat/BUILD/gcc-4.5.2/host-x86_64-unknown-linux-gnu/gcc'
make[2]: *** [all-stage2-gcc] Error 2
make[2]: Leaving directory `/usr/src/redhat/BUILD/gcc-4.5.2'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/usr/src/redhat/BUILD/gcc-4.5.2'
make: *** [bootstrap] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.35144 (%build)
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-17 14:36:18 PDT
Created attachment 553913 [details] [diff] [review]
updated patch

This one also backports a fix for correctly handling size types. A bootstrap in 64 bits is currently in stage3.

I will upload the rpms once it finishes bootstrapping both in 32 and 64 bits and building firefox with them.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-17 16:32:16 PDT
Both bootstraps and firefox builds have finished. The rpms are in
http://people.mozilla.org/~respindola/gcc/
Comment 7 Chris AtLee [:catlee] 2011-08-18 09:08:49 PDT
Created attachment 554109 [details] [diff] [review]
deploy gcc test packages
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-20 20:37:41 PDT
A try run with the patched gcc is at

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=425c499fae7a
Comment 9 Ed Morley [:emorley] 2011-08-21 05:33:56 PDT
Try build in comment 8 failed on Windows, OS X and Maemo.

Removing checkin-needed for now.
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-21 07:57:18 PDT
Created attachment 554717 [details] [diff] [review]
pr676607.patch

Update gcc for real.
Comment 11 Ed Morley [:emorley] 2011-08-21 08:05:20 PDT
The comment 10 patch doesn't include the file pr49911.diff.
Comment 12 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 06:40:45 PDT
(In reply to Ed Morley [:edmorley] from comment #9)
> Try build in comment 8 failed on Windows, OS X and Maemo.

They don't use our gcc. My bad for enabling try on targets that were it didn't made sense.

> Removing checkin-needed for now.

Thanks. Can I use that flag only on the attachments? That way it gets cleared automatically on the versions.
Comment 13 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-22 06:42:31 PDT
Created attachment 554844 [details] [diff] [review]
new patch

Now with pr49911.diff included.
Comment 14 Ed Morley [:emorley] 2011-08-22 06:52:39 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #12)
> They don't use our gcc. My bad for enabling try on targets that were it
> didn't made sense.

Indeed they don't, but without digging into that try diff vs the one requested for checkin, it wasn't clear whether it was going to break things when it landed. I've only had level 3 commit privs for 2 days now, so wasn't inclined to risk it :-)

> > Removing checkin-needed for now.
> 
> Thanks. Can I use that flag only on the attachments? That way it gets
> cleared automatically on the versions.

The flag on attachments is newish, so some people who push checkin-needed patches are still using old saved searches, that don't show it (rather than searches that show either/or). Myself and a few others are using the new search though, so it will still get picked up eventually. It's up to you which you use :-)
Comment 15 Ben Hearsum (:bhearsum) 2011-08-22 14:25:58 PDT
Comment on attachment 554844 [details] [diff] [review]
new patch

The patch looks fine but I don't really know the details of where/how catlee has been generating these RPMs. This doesn't seem urgent, so I'm not going to try to figure it out.
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 06:48:43 PDT
This blocks 669953 which is fairly important to get faster test results. It also blocks a full switch to clang on OS X as clang's -O0 is fast but produces worse code than gcc's.

Given the gcc bugs were have found in bug 681054, that is getting more and more important.

So while it is true this is not "urgent", it is seems to be more important than how it has been handled. It is bad that this has been open for 19 days with week long intervals without feedback from releng.
Comment 17 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 06:59:46 PDT
Note that I am not familiar with mozilla's infrastructure yet, but I have done some production work in previous jobs.

Debugging the gcc issue took a day, getting the first reply to this bug took 13, so it might be productive to put me in the team until these interactions are done.
Comment 18 Chris Cooper [:coop] [away until Aug 29] 2011-08-23 07:27:26 PDT
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #17)
> Note that I am not familiar with mozilla's infrastructure yet, but I have
> done some production work in previous jobs.
> 
> Debugging the gcc issue took a day, getting the first reply to this bug took
> 13,

We're not trying to stall you here, honest. 

People go on vacation. It happens (especially in August). This is catlee's bug, and honestly, he has much more experience with rpm than most others on the team. Everyone else in releng has other work they're involved in. If someone else has to context-switch to pick this up, something else gets dropped or pushed out.

> so it might be productive to put me in the team until these interactions
> are done.

Can we avoid these types of suggestions, please? 

I understand your desire to see this fixed, but it belittles the efforts of your co-workers in releng. To pick-up and understand our entire system/infrastructure in 4 days and land a compiler change in a way that doesn't break *something* would be an impressive feat. 

I will talk to joduinn about the relative importance of this particular fix vs. other things already in the pipe. I also understand the Armen has offered to get you machines to do the final bits of testing that are required. Worst case scenario is that catlee picks this up on Monday when he gets back.
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 08:05:37 PDT
The machines are for future testing. This patch has been built, deployed to the actual bots and a try run has been done.

I don't think anyone is trying to stall me, but when you have a few gatekeepers it is unavoidable that they would become overloaded. The suggestion is an honest one. This will not me my last iteration with releng and I do have experience doing production jobs before.
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-23 08:12:59 PDT
Created attachment 555106 [details] [diff] [review]
Alternative patch, use the already deployed compiler.

Hello Armen,

This is the patch I described over IRC. Instead of building what is essentially the same rpm again, we can just use the one catlee deployed already.

It does look funny to use a path with "-test" in it, but we can fix that afterwards and it would not be in the critical path.
Comment 21 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-08-25 12:04:53 PDT
With regards the patch I would start first with mozilla-inbound giving a heads up and then within a day everywhere else. We need to get proper test coverage.
We should not touch mozilla-aurora, mozilla-beta and mozilla-release without approvals.

Would we see any performance improvements?

espindola can you run this mozconfig change through the try server to give me better piece of mind?

Sounds good?

This is the output of the build with gcc-4.5-test:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1314289116.1314293492.19213.gz&fulltext=1

Here is the mozconfig:
ac_add_options --enable-application=browser
ac_add_options --enable-optimize
ac_add_options --enable-update-channel=nightly
ac_add_options --enable-update-packaging
ac_add_options --disable-debug
ac_add_options --enable-tests
ac_add_options --enable-codesighs

# Nightlies only since this has a cost in performance
ac_add_options --enable-js-diagnostics

CC=/tools/gcc-4.5-test/bin/gcc
CXX=/tools/gcc-4.5-test/bin/g++
# Avoid dependency on libstdc++ 4.5
ac_add_options --enable-stdcxx-compat

export CFLAGS="-gdwarf-2"
export CXXFLAGS="-gdwarf-2"

# For NSS symbols
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debug-symbols="-gdwarf-2"

# Needed to enable breakpad in application.ini
export MOZILLA_OFFICIAL=1

export MOZ_TELEMETRY_REPORTING=1

# PGO
mk_add_options MOZ_PGO=1
mk_add_options PROFILE_GEN_SCRIPT='$(PYTHON) @MOZ_OBJDIR@/_profile/pgo/profileserver.py 10'

# Enable parallel compiling
mk_add_options MOZ_MAKE_FLAGS="-j4"

#Use ccache
ac_add_options --with-ccache=/usr/bin/ccache
Comment 22 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-25 14:22:49 PDT
(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #21)
> With regards the patch I would start first with mozilla-inbound giving a
> heads up and then within a day everywhere else. We need to get proper test
> coverage.
> We should not touch mozilla-aurora, mozilla-beta and mozilla-release without
> approvals.

That is probably a good idea. In fact, I would argue that this is probably a better way to update compilers in general: Never overwrite the old one, just install a new one and then change the mozconfigs.

> Would we see any performance improvements?

Unexpected. This is just a correctness fix.
 
> espindola can you run this mozconfig change through the try server to give
> me better piece of mind?

Sure:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=be97c9e34ec8

That run also includes the configure check for broken compilers.
Comment 23 Chris AtLee [:catlee] 2011-09-07 06:00:40 PDT
Created attachment 558781 [details] [diff] [review]
install gcc-4.5-0moz2 package
Comment 24 Chris AtLee [:catlee] 2011-09-07 06:03:57 PDT
Created attachment 558782 [details] [diff] [review]
build gcc-4.5-0moz2 into /tools/gcc-4.5-0moz2
Comment 25 Chris AtLee [:catlee] 2011-09-07 06:18:46 PDT
Comment on attachment 555106 [details] [diff] [review]
Alternative patch, use the already deployed compiler.

please use /tools/gcc-4.5-0moz2/bin/gcc
Comment 26 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-22 11:34:43 PST
A try with the new compiler is at
https://tbpl.mozilla.org/?tree=Try&rev=ef3c7d2fd830
Comment 27 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-22 14:23:37 PST
In https://tbpl.mozilla.org/php/getParsedLog.php?id=7536237&tree=Try&full=1

checking for gcc PR49911... no

in https://tbpl.mozilla.org/php/getParsedLog.php?id=7535923&tree=Try&full=1

checking for gcc PR49911... yes
Comment 28 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-22 14:24:48 PST
Created attachment 576290 [details] [diff] [review]
update the mozconfigs
Comment 29 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-23 06:09:50 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3a5fd06fa0
Comment 30 Matt Brubeck (:mbrubeck) 2011-11-23 10:14:28 PST
https://hg.mozilla.org/mozilla-central/rev/ac3a5fd06fa0

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