676607 - Patch our gcc 4.5 to fix gcc's PR49911

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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)]

Attachment: 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)]

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)]

The two builds are done, is the patch OK?

Comment 4

[Chris AtLee [:catlee]]

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)]

Attachment: 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)]

Both bootstraps and firefox builds have finished. The rpms are in
http://people.mozilla.org/~respindola/gcc/

Comment 7

[Chris AtLee [:catlee]]

Attachment: deploy gcc test packages

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

A try run with the patched gcc is at

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

Comment 9

[Ed Morley [:emorley]]

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)]

Attachment: pr676607.patch

Update gcc for real.

Comment 11

[Ed Morley [:emorley]]

The comment 10 patch doesn't include the file pr49911.diff.

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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)]

Attachment: new patch

Now with pr49911.diff included.

Comment 14

[Ed Morley [:emorley]]

(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

[[e-mail] (: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.

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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)]

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] (he/him)]

(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)]

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)]

Attachment: 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 [: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

Comment 22

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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]]

Attachment: install gcc-4.5-0moz2 package

Comment 24

[Chris AtLee [:catlee]]

Attachment: build gcc-4.5-0moz2 into /tools/gcc-4.5-0moz2

Comment 25

[Chris AtLee [:catlee]]

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)]

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)]

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)]

Attachment: update the mozconfigs

Comment 29

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3a5fd06fa0

Comment 30

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ac3a5fd06fa0