Closed
Bug 1120060
Opened 8 years ago
Closed 8 years ago
Remove gcc 4.5.0 workaround for PGO crash in ConvolveHorizontally()
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: cpeterson, Assigned: tejas.srinivasan95, Mentored)
References
Details
(Whiteboard: [gfx-noted][good first bug][lang=c++])
Attachments
(1 file, 1 obsolete file)
1.43 KB,
patch
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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
Updated•8 years ago
|
Mentor: tnikkel
Reporter | ||
Updated•8 years ago
|
Whiteboard: gfx-noted → [gfx-noted][good first bug][lang=c++]
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
(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.
Assignee | ||
Comment 4•8 years ago
|
||
Kindly review the patch I have attached. Thanks.
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
(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•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8548003 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Made the requisite commit message changes.
Attachment #8548254 -
Flags: checkin?
Comment 9•8 years ago
|
||
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?
Updated•8 years ago
|
Assignee: nobody → tejas.srinivasan95
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda146977dfe
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
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.
Reporter | ||
Comment 12•8 years ago
|
||
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 14•8 years ago
|
||
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.
Description
•