Summary: | Generators violate bytecode liveness validation | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, keith_miller, mark.lam, msaboff, rniwa, saam, ysuzuki | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=159281 | ||||||
Attachments: |
|
Description
Filip Pizlo
2016-06-29 15:01:54 PDT
I'm going to try to fix this. Looks like this is because the liveness analysis for generators is unsound with respect to try/catch/finally. I think this is more proof that we should not use a single liveness analysis for both pre-generator-converstion and post-generator-conversion bytecode. It's too confusing! For example, resume must preserve the invariant that it defines everything and only uses its token argument. But that's not what happens if it's inside a try block. In order to support try, we assume that any bytecode inside a try uses anything that the catch uses. I think that the best way to go is: 1) Write a custom liveness analysis just for the generator conversion. This custom analysis could reuse BytecodeUseDef.h and the basic block analysis, but otherwise it will be a new thing. This allows it to play games with save/resume. For example, it allows it to implement these rules easily: (a) the live-in at resume is always just the token argument and nothing else, and (b) whatever was live-out at the resume is used by the corresponding save. 2) Write a transformation that gets rid of save/resume and makes the control and data flow explicit. 3) Remove any "generator" modes or tricks from the bytecode liveness analysis that the rest of the engine uses. I think that for now, I'll implement a hack that says that resume is exempted from catch liveness. That's sort of insane and hacky, but I think it will fix this bug for now. Created attachment 282390 [details]
the patch
(In reply to comment #2) > Looks like this is because the liveness analysis for generators is unsound > with respect to try/catch/finally. Oops! > > I think this is more proof that we should not use a single liveness analysis > for both pre-generator-converstion and post-generator-conversion bytecode. > It's too confusing! Right. > > For example, resume must preserve the invariant that it defines everything > and only uses its token argument. But that's not what happens if it's > inside a try block. In order to support try, we assume that any bytecode > inside a try uses anything that the catch uses. > > I think that the best way to go is: > > 1) Write a custom liveness analysis just for the generator conversion. This > custom analysis could reuse BytecodeUseDef.h and the basic block analysis, > but otherwise it will be a new thing. This allows it to play games with > save/resume. For example, it allows it to implement these rules easily: (a) > the live-in at resume is always just the token argument and nothing else, > and (b) whatever was live-out at the resume is used by the corresponding > save. > > 2) Write a transformation that gets rid of save/resume and makes the control > and data flow explicit. > > 3) Remove any "generator" modes or tricks from the bytecode liveness > analysis that the rest of the engine uses. > > I think that for now, I'll implement a hack that says that resume is > exempted from catch liveness. That's sort of insane and hacky, but I think > it will fix this bug for now. sounds nice. generatorification should remove these resume / save and the usual liveness analysis should not care about them. after landing isNaN / isFinite patch, go to the generator patch, it is now introducing bytecode analysis. Comment on attachment 282390 [details]
the patch
r+
(In reply to comment #4) > sounds nice. generatorification should remove these resume / save and the > usual liveness analysis should not care about them. > after landing isNaN / isFinite patch, go to the generator patch, it is now > introducing bytecode analysis. oops, bad syntax. (sorry, now typing from my phone) After landing the isNaN / isFinite patch, I'll go to the generatorification patch, which should include the separated generatorification analysis from the usual bytecode liveness analysis. Landed in https://trac.webkit.org/changeset/202689 |