Closed
Bug 1028629
Opened 10 years ago
Closed 10 years ago
Remove a function pointer (encodeVFP)
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file, 1 obsolete file)
1.73 KB,
patch
|
sfink
:
review+
lmandel
:
approval-mozilla-b2g32+
|
Details | Diff | Splinter Review |
The encodeVFP function pointer confuses the rooting hazard analysis.
Assignee | ||
Comment 1•10 years ago
|
||
This makes the hazard analysis happier, and doesn't seem to be any less clear. Gives the optimizer a tiny bit less work to do, too. :-)
Attachment #8444026 -
Flags: review?(mrosenberg)
Comment 2•10 years ago
|
||
Comment on attachment 8444026 [details] [diff] [review]
Remove a function pointer (encodeVFP)
Review of attachment 8444026 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/arm/Assembler-arm.cpp
@@ +2108,1 @@
> }
I think you wanted to call VN/VM?
Attachment #8444026 -
Flags: review?(mrosenberg)
Assignee | ||
Comment 3•10 years ago
|
||
And it still compiles! Sorry about that.
Attachment #8444508 -
Flags: review?(mrosenberg)
Assignee | ||
Updated•10 years ago
|
Attachment #8444026 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)
r=mjrosenb via IRC
Attachment #8444508 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling the hazard analysis on b2g, bug 898554
User impact if declined: none
Testing completed: it's been in mozilla-central for a month
Risk to taking this patch (and alternatives if risky): very very low
String or UUID changes made by this patch: none
Approval Request Comment
[Feature/regressing bug #]: b2g hazard analysis, bug 898554
[User impact if declined]: none
[Describe test coverage new/current, TBPL]: mozilla-central for a month
[Risks and why]: very very low. See below.
[String/UUID change made/needed]: none
This is really a hygiene thing. Right now, aurora and b2g32 both report 2 rooting hazards, and I have the threshold set to 4 (so the job will turn red if more than 4 hazards are detected.) The 2 hazards are false positives that this patch would fix. There is no reason not to lower the threshold to 2, so that any added hazard would turn the build red. But if that happened, then the person landing the patch would see their new hazard mixed in with 2 others and wouldn't necessarily know which one needed to be fixed. It would be better to land this trivial patch to drop the hazards to zero, along with a patch to set the expected number of hazards to zero, so that any added hazards are immediately highlighted.
And yes, I should have done this in the first place, instead of waiting for the uplift. Sorry! I hadn't realized these were still there.
Attachment #8444508 -
Flags: approval-mozilla-b2g32?
Attachment #8444508 -
Flags: approval-mozilla-aurora?
Comment 8•10 years ago
|
||
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)
This landed on 33, which is what Aurora is now. Unless beta needs this for Android, you should be good with just b2g32.
Attachment #8444508 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
status-firefox32:
--- → affected
status-firefox33:
--- → fixed
Comment 9•10 years ago
|
||
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)
As the bug shows, this patch has been on m-c for 3 weeks and is quite small and looks to be pretty trivial. I'm going to take this small hygiene patch for 2.0.
Attachment #8444508 -
Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Comment 10•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•