Patch our gcc 4.5 to fix gcc's PR49911

RESOLVED FIXED

Status

Release Engineering
General
P4
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

other
x86_64
Linux

Firefox Tracking Flags

(firefox11 fixed)

Details

(Whiteboard: [qa-], URL)

Attachments

(5 attachments, 5 obsolete attachments)

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
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.
Attachment #550765 - Flags: review?(catlee)
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.
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?

Updated

6 years ago
Attachment #552248 - Flags: review?(catlee) → review+
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → catlee
Priority: -- → P4
Blocks: 678558
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)
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.
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/
Created attachment 554109 [details] [diff] [review]
deploy gcc test packages
Attachment #554109 - Flags: review?(bear)

Updated

6 years ago
Attachment #554109 - Flags: review?(bear) → review+

Updated

6 years ago
Attachment #554109 - Flags: checked-in+
A try run with the patched gcc is at

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

Comment 9

6 years ago
Try build in comment 8 failed on Windows, OS X and Maemo.

Removing checkin-needed for now.
Keywords: checkin-needed
Created attachment 554717 [details] [diff] [review]
pr676607.patch

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.
Created attachment 554844 [details] [diff] [review]
new patch

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.
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.
Attachment #555106 - Flags: review?(armenzg)

Comment 21

6 years ago
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.
No longer blocks: 678558
No longer blocks: 669953
Attachment #555106 - Flags: review?(armenzg) → review?(catlee)
Created attachment 558781 [details] [diff] [review]
install gcc-4.5-0moz2 package
Attachment #558781 - Flags: review?(bhearsum)
Created attachment 558782 [details] [diff] [review]
build gcc-4.5-0moz2 into /tools/gcc-4.5-0moz2
Attachment #558782 - Flags: review?(bhearsum)

Updated

6 years ago
Attachment #554844 - Attachment is obsolete: true
Attachment #558782 - Flags: review?(bhearsum) → review+
Attachment #558781 - Flags: review?(bhearsum) → review+

Updated

6 years ago
Attachment #558782 - Flags: checked-in+

Updated

6 years ago
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-

Updated

6 years ago
Assignee: catlee → respindola
A try with the new compiler is at
https://tbpl.mozilla.org/?tree=Try&rev=ef3c7d2fd830
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
Created attachment 576290 [details] [diff] [review]
update the mozconfigs
Attachment #576290 - Flags: review?(catlee)

Updated

6 years ago
Attachment #576290 - Flags: review?(catlee) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3a5fd06fa0
https://hg.mozilla.org/mozilla-central/rev/ac3a5fd06fa0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox11: --- → fixed
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.