Created attachment 745846 [details] [diff] [review] Patch (In reply to Luke Wagner [:luke] from bug 868437 comment 3) > Also, we should totally rip out CallArgsList. I added it because the > Debugger folks said they needed it "yesterday" so that the debugger stack > walking could expose calls to builtins, but it is 2 years now and that > hasn't happened, so let's take it out. The attached patch does this, and I was able to simplify quite a lot of code. 11 files changed, 129 insertions(+), 442 deletions(-) With this patch StackIter only iterates script frames, so it's now identical to ScriptFrameIter. I can post a follow-up patch to remove ScriptFrameIter, or rename StackIter to ScriptFrameIter. Any preference for StackIter vs. ScriptFrameIter?
Attachment #745846 - Flags: review?(luke)
Comment on attachment 745846 [details] [diff] [review] Patch Looks great, thanks for doing this!
Attachment #745846 - Flags: review?(luke) → review+
At some point, it'd be nice to take invoke args off the stack altogether and use AutoValueVector. Right now we can't because the incoming arg array to js::Invoke is used as the arguments to a scripted stack frame. However, if we do the simplification that the callee (Interpret, baseline, ion) pushes their own frame (which means copying the given argument array to some callee-appropriate storage), then I think we could.
(In reply to Jan de Mooij [:jandem] from comment #0) > Any preference for StackIter vs. ScriptFrameIter? ScriptFrameIter seems more precise.
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f778d03364 Green Linux/Windows Try: https://tbpl.mozilla.org/?tree=Try&rev=5a3ea59c40f7
Whiteboard: [leave open]
Follow-up to fix --disable-ion red: https://hg.mozilla.org/integration/mozilla-inbound/rev/920cea554b7f
Created attachment 746267 [details] [diff] [review] Part 2: rm ScriptFrameIter, s/StackIter/ScriptFrameIter Removes ScriptFrameIter and renames StackIter to ScriptFrameIter.
Attachment #746267 - Flags: review?(luke)
Backed out for breaking Windows PGO builds. https://hg.mozilla.org/integration/mozilla-inbound/rev/e39de8eb4716 https://tbpl.mozilla.org/php/getParsedLog.php?id=22686831&tree=Mozilla-Inbound
Comment on attachment 745846 [details] [diff] [review] Patch gkw, decoder: would you guys mind fuzzing this patch for a bit? It's green on TBPL but for some reason it breaks Win32 PGO builds. Please either use this patch or inbound revision 93f778d03364. Thanks!
Comment on attachment 745846 [details] [diff] [review] Patch Nothing found so far :)
Attachment #745846 - Flags: feedback?(choller) → feedback+
Yeah, this is a MSVC PGO bug. With just the changes to StackIter::settleOnNewState, I can reproduce it locally and on Try. A PGO-instrumented xpcshell crashes on startup, while creating the first JSContext. It doesn't touch StackIter before it crashes and the (instrumented) JS shell runs jit-tests without problem. Disabling PGO for settleOnNewState fixes the crash: https://tbpl.mozilla.org/?tree=Try&rev=de762042d2d1 (that's a PGO build). Relanded part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/77141947c30f
Comment on attachment 745846 [details] [diff] [review] Patch Nothing bad found, tested on Mac and Windows 32-bit.
Attachment #745846 - Flags: feedback?(gary) → feedback+
And part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6a6fe7ccf6 Gary and Christian, thanks for the fuzzing help!
Whiteboard: [leave open]
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.