Remove gcc 4.5.0 workaround for PGO crash in ConvolveHorizontally()

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: tejas.srinivasan95, Mentored)

Tracking

unspecified
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gfx-noted][good first bug][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

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
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.
Whiteboard: gfx-noted
Mentor: tnikkel
Whiteboard: gfx-noted → [gfx-noted][good first bug][lang=c++]
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?
(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.
Posted patch removed_workaround.patch (obsolete) — Splinter Review
Kindly review the patch I have attached. Thanks.
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.
Attachment #8548003 - Flags: review+
(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.
(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.
Attachment #8548003 - Attachment is obsolete: true
Made the requisite commit message changes.
Attachment #8548254 - Flags: checkin?
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!
Attachment #8548254 - Flags: checkin?
Assignee: nobody → tejas.srinivasan95
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
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.
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
https://hg.mozilla.org/mozilla-central/rev/fda146977dfe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Thanks a lot everyone! Feels good to have one bug under my belt. :)
You need to log in before you can comment on or make changes to this bug.