-
-
Notifications
You must be signed in to change notification settings - Fork 925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Also perform uninitialized check on temp locals. #4252
Conversation
@subbuss @headius I think our issue in this specific case is the optarg is missing an init when it is an lvar and then we convert it to a temp. So this fixes the problem but I think the underlying issue is we convert non-nil-corrected lvars into temp vars. Another argument worth asking is should we worry about keeping this assumption that temp vars are always initialized or should we just add this extra logic and cover our asses? |
This patch is incomplete. I need to page in this code into my memory. But, looks like I started doing the initialization for all vars (see buildDataflowVars which gets all vars, not just LocalVariables), and at some point decided to test for inits for only LocalVariables. So, anywhere there is a check for LocalVariable in the code needs an update to not assume lvar. I can follow up if you want, or I can point you to other places to fix. applyTransferFunction is the only other place it seems .. since tmps don't cross scope boundaries, identifyUndefinedVarsInClosure is correct to look at lvars. |
You can reproduce the error with this snippet as well:
You will find that foo will fail to JIT with the same error. This is because we run the the AddMissingInitsPass at the end to catch all possible scenarios of missing inits (which is why the code should work for tmps and lvars). In this case, the OptimizeDynamicVarsPass converts all the lvars to tmps and the tmps will have a missing init instead of the lvar. So, your patch is on the right track. You just need to fix applyTransferFunction as well. |
@subbuss I am agreeing it is better to have this work on all variable types now since we have hit this problem more than once; but if we move missing inits higher up the pass chain your snippet will not be broken anymore. As a point of general guidance should all passes be made to work in any order as a principle do you think? Or should we just get better about enforcing dependencies? |
Both.
|
Great! I'll work on that once we're back from Austin. |
67c0c7d
to
e50ec79
Compare
I added the fix for |
e50ec79
to
fe0157d
Compare
You will hate me, but now that I look at the code again, I was wrong. tmp vars don't leak in from outer scopes. Only lvars do! So, applyTransferFunction was correct as is. So, your previous fix was indeed complete. Sorry, but I seem to mix up this "simple" tmp / lvar distinction every once in a while. |
@subbuss Oh, so like the closure var walker later in the file, this should only ever see |
Yes. Can you please add JIT tests (either as part of the PR or in a followup commit)? The example I added earlier in #4252 (comment) should be good enough. |
fe0157d
to
061764e
Compare
@subbuss This was already failing a couple JIT tests, but I'll add any cases that aren't covered. |
Fixes #4251.