Closed Bug 676607 Opened 9 years ago Closed 8 years ago

Patch our gcc 4.5 to fix gcc's PR49911

Categories

(Release Engineering :: General, defect, P4)

x86_64
Linux
defect

Tracking

(firefox11 fixed)

RESOLVED FIXED
Tracking Status
firefox11 --- fixed

People

(Reporter: espindola, Assigned: espindola)

References

()

Details

(Whiteboard: [qa-])

Attachments

(5 files, 5 obsolete files)

2.91 KB, patch
bear
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
143.13 KB, patch
catlee
: review-
Details | Diff | Splinter Review
1.71 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
10.69 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
10.35 KB, patch
catlee
: review+
Details | Diff | Splinter Review
Attached patch proposed patch (obsolete) — Splinter Review
With this patch I am able to creating a working firefox debug build with optimizations on 32 bit centos.
Attachment #550765 - Flags: review?(catlee)
Attached patch New patch I am testing (obsolete) — Splinter Review
As discussed on IRC, this patch changes the name and install dir so that it can be installed in parallel for testing.
Attachment #550765 - Attachment is obsolete: true
Attachment #550765 - Flags: review?(catlee)
Attachment #552248 - Flags: review?(catlee)
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?
The two builds are done, is the patch OK?
Attachment #552248 - Flags: review?(catlee) → review+
Assignee: nobody → catlee
Priority: -- → P4
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)
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #552248 - Attachment is obsolete: true
Attachment #553913 - Flags: review?
Both bootstraps and firefox builds have finished. The rpms are in
http://people.mozilla.org/~respindola/gcc/
Attachment #554109 - Flags: review?(bear)
Attachment #554109 - Flags: review?(bear) → review+
Attachment #554109 - Flags: checked-in+
Try build in comment 8 failed on Windows, OS X and Maemo.

Removing checkin-needed for now.
Keywords: checkin-needed
Attached patch pr676607.patch (obsolete) — Splinter Review
Update gcc for real.
Attachment #553913 - Attachment is obsolete: true
Attachment #553913 - Flags: review?
Attachment #554717 - Flags: review?(catlee)
The comment 10 patch doesn't include the file pr49911.diff.
(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.
Attached patch new patch (obsolete) — Splinter Review
Now with pr49911.diff included.
Attachment #554717 - Attachment is obsolete: true
Attachment #554717 - Flags: review?(catlee)
Attachment #554844 - Flags: review?(catlee)
(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 :-)
Attachment #554844 - Flags: review?(catlee) → review?(bhearsum)
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.
Attachment #554844 - Flags: review?(bhearsum) → review+
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.
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.
(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.
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.
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.
Attachment #555106 - Flags: review?(armenzg)
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
(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.
Attachment #555106 - Flags: review?(armenzg) → review?(catlee)
Attachment #558781 - Flags: review?(bhearsum)
Attachment #554844 - Attachment is obsolete: true
Attachment #558782 - Flags: review?(bhearsum) → review+
Attachment #558781 - Flags: review?(bhearsum) → review+
Attachment #558782 - Flags: checked-in+
Attachment #558781 - Flags: checked-in+
Comment on attachment 555106 [details] [diff] [review]
Alternative patch, use the already deployed compiler.

please use /tools/gcc-4.5-0moz2/bin/gcc
Attachment #555106 - Flags: review?(catlee) → review-
Assignee: catlee → respindola
Attachment #576290 - Flags: review?(catlee) → review+
https://hg.mozilla.org/mozilla-central/rev/ac3a5fd06fa0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.