bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

rm CallArgsList, StackIter native call support

RESOLVED FIXED in mozilla23



JavaScript Engine
5 years ago
5 years ago


(Reporter: jandem, Assigned: jandem)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments)



5 years ago
Created attachment 745846 [details] [diff] [review]

(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 1

5 years ago
Comment on attachment 745846 [details] [diff] [review]

Looks great, thanks for doing this!
Attachment #745846 - Flags: review?(luke) → review+

Comment 2

5 years ago
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.

Comment 3

5 years ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> Any preference for StackIter vs. ScriptFrameIter?

ScriptFrameIter seems more precise.

Comment 5

5 years ago
Follow-up to fix --disable-ion red:


Comment 6

5 years ago
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)

Comment 8

5 years ago
Comment on attachment 745846 [details] [diff] [review]

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!
Attachment #745846 - Flags: feedback?(gary)
Attachment #745846 - Flags: feedback?(choller)
Depends on: 869544
Depends on: 869546


5 years ago
Attachment #746267 - Flags: review?(luke) → review+
Comment on attachment 745846 [details] [diff] [review]

Nothing found so far :)
Attachment #745846 - Flags: feedback?(choller) → feedback+

Comment 11

5 years ago
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:

Comment on attachment 745846 [details] [diff] [review]

Nothing bad found, tested on Mac and Windows 32-bit.
Attachment #745846 - Flags: feedback?(gary) → feedback+

Comment 13

5 years ago
And part 2:


Gary and Christian, thanks for the fuzzing help!
Whiteboard: [leave open]
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.