1120060 - Remove gcc 4.5.0 workaround for PGO crash in ConvolveHorizontally()

Status: RESOLVED FIXED

Description

[Chris Peterson [:cpeterson]]

Support for gcc 4.5 was removed in bug 1077549, so we should be able to backout this gcc 4.5 workaround:

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

Comment 1

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

This would make a good first bug for contributors assuming there's no complications with removing this workaround. tn, leaving this to you to back out or mentor as you choose.

Comment 2

[Tejas Srinivasan]

Hi I'd like to work on this bug. Do I just need to remove the workaround code introduced in the patch for Bug 827946? Anything to build in particular before getting started? Any tests I have to run?

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Tejas Srinivasan from comment #2)
> Do I just need to remove the workaround
> code introduced in the patch for Bug 827946?

Yep.

> Anything to build in particular
> before getting started? Any tests I have to run?

Normally you'd want to get a build of Firefox going first, but you could probably get away without doing that in this case. Any you shouldn't need to run any tests, this is the kind of change that is correct if it compiles cleanly.

Comment 4

[Tejas Srinivasan]

Attachment: removed_workaround.patch

Kindly review the patch I have attached. Thanks.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8548003 [details] [diff] [review]
removed_workaround.patch

Looks good.

You should also mention gcc 4.5 in the commit message, and that we no longer need the work around because gcc4.5 is now deprecated.

Next time you want someone to review a patch it's best to set the review? flag on the patch and put a specific person in the box. Otherwise the review request is most likely to not be noticed. Setting the review? flag and tagging a specific person means they get an email, and it shows up on their review queue.

Thanks.

Comment 6

[Tejas Srinivasan]

(In reply to Timothy Nikkel (:tn) from comment #5)
> Comment on attachment 8548003 [details] [diff] [review]
> removed_workaround.patch
> 
> Looks good.

Thanks.

> You should also mention gcc 4.5 in the commit message, and that we no longer
> need the work around because gcc4.5 is now deprecated.

Sorry, I should have included that in the commit message.

> Next time you want someone to review a patch it's best to set the review?
> flag on the patch and put a specific person in the box. Otherwise the review
> request is most likely to not be noticed. Setting the review? flag and
> tagging a specific person means they get an email, and it shows up on their
> review queue.

I did set the review? flag and put you as the requestee. However, I got a message saying that I didn't have access to you or something like that, so I just removed the review? flag. Sorry for the inconvenience.

How do I get the patch checked in to the repository? This is my first bug, so not quite sure how this works.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Tejas Srinivasan from comment #6)
> > You should also mention gcc 4.5 in the commit message, and that we no longer
> > need the work around because gcc4.5 is now deprecated.
> 
> Sorry, I should have included that in the commit message.

No problem, that's what review is for.

> > Next time you want someone to review a patch it's best to set the review?
> > flag on the patch and put a specific person in the box. Otherwise the review
> > request is most likely to not be noticed. Setting the review? flag and
> > tagging a specific person means they get an email, and it shows up on their
> > review queue.
> 
> I did set the review? flag and put you as the requestee. However, I got a
> message saying that I didn't have access to you or something like that, so I
> just removed the review? flag. Sorry for the inconvenience.

I guess you need a permission bit set on your account to be able to do that, my mistake. I'll see if I can get that set for you.

> How do I get the patch checked in to the repository? This is my first bug,
> so not quite sure how this works.

Upload a new patch to this bug with the updated commit message, marking the old patch obsolete. The add the "checkin-needed" keyword to the keywords field.

Comment 8

[Tejas Srinivasan]

Attachment: removed_workaround.patch

Made the requisite commit message changes.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8548254 [details] [diff] [review]
removed_workaround.patch

For future reference, the checkin-needed bug keyword is the preferred way for requesting checkin since it works better with our automated bug marking tools. Thanks for the patch!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Timothy Nikkel (:tnikkel)]

Tejas, apparently this is how you get permission bits set on your bugzilla account
https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html

Looks like you need three bugs first, let me know if you need any help.

Comment 12

[Chris Peterson [:cpeterson]]

Thanks, Tejas! I was going suggest you look at the "Bugs Ahoy" website [1] to find another bug that interests you, but I see that you are already looking at bug 1120652. :)

[1] http://www.joshmatthews.net/bugsahoy/?cpp=1&unowned=1&simple=1

Comment 13

[Wes Kocher (:KWierso)]

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

Comment 14

[Tejas Srinivasan]

Thanks a lot everyone! Feels good to have one bug under my belt. :)