Remove flow sensitive alias analysis option

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: mgaudet, Assigned: Himanshuteli, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla62
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In bug 1455280 flow sensitive alias analysis was removed, but the option was left behind to simplify fuzzing and bisection. Nevertheless, at some point the option should go away.
(Assignee)

Comment 1

8 months ago
I would like to work on this can you guide me
Hey, I'd love to guide you. 

First step: make sure you get a copy of the sources, and are able to build at least spidermonkey [1]. In my experience, using bootstrap.py is the easiest way to get ready to go [2].  

So the option used to be 'ion-aa'. A very useful tool for you is https://searchfox.org/, which will help you find the references to that option. If I recall correctly there are two of them. You'll just need to prepare a patch to excise them. 

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation
(Assignee)

Comment 3

8 months ago
I have successfully build firefox and compiled it.
If I'm correct, we need to remove the lines: 
if (op.getStringOption("ion-aa")) {}
|| !op.addStringOption('\0', "ion-aa", "flow-sensitive/flow-insensitive",
                               "Specify wheter or not to use flow sensitive Alias Analysis"
                               "(default: flow-insensitive)")
located in js/src/shell/js.cpp

Let's wait for more info.
Those look about right to me. So now, you'll do the removal, ensure things still build, then upload a patch to this bug.
(Assignee)

Comment 5

8 months ago
Created attachment 8975864 [details] [diff] [review]
js.cpp

Removed flow sensitive alias analysis option

Comment 6

8 months ago
(In reply to Himanshuteli from comment #5)
> Created attachment 8975864 [details] [diff] [review]
> js.cpp
> 
> Removed flow sensitive alias analysis option

Please join #introduction on irc.mozilla.org, then I will help you create a patch.
(Assignee)

Comment 8

8 months ago
Created attachment 8979027 [details] [diff] [review]
1460883-Remove-flow-sensitive-alias-analysis-option

Removed flow sensitive alias analysis option. I hope this patch is correct.
Attachment #8975864 - Attachment is obsolete: true
Assignee: nobody → Himanshuteli
Comment on attachment 8979027 [details] [diff] [review]
1460883-Remove-flow-sensitive-alias-analysis-option

Review of attachment 8979027 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! :)
Attachment #8979027 - Flags: review+
If you are happy with the patch, add "checkin-needed" to the |Keywords| list (which you can edit by clicking Edit Bug near the top of page). A Sheriff will add my name to the commit message and checkin the patch for us.
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 11

8 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5184707be08
Remove flow sensitive alias analysis option r=tcampbell
Keywords: checkin-needed

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5184707be08
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
\o/ Congrats on landing your first SpiderMonkey patch!
You need to log in before you can comment on or make changes to this bug.