868990 - rm CallArgsList, StackIter native call support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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?

Comment 1

[Luke Wagner [:luke]]

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

Looks great, thanks for doing this!

Comment 2

[Luke Wagner [:luke]]

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

[Luke Wagner [:luke]]

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

ScriptFrameIter seems more precise.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f778d03364

Green Linux/Windows Try: https://tbpl.mozilla.org/?tree=Try&rev=5a3ea59c40f7

Comment 5

[Jan de Mooij [:jandem]]

Follow-up to fix --disable-ion red:

https://hg.mozilla.org/integration/mozilla-inbound/rev/920cea554b7f

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2: rm ScriptFrameIter, s/StackIter/ScriptFrameIter

Removes ScriptFrameIter and renames StackIter to ScriptFrameIter.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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 8

[Jan de Mooij [:jandem]]

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 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/920cea554b7f

Comment 10

[Christian Holler (:decoder)]

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

Nothing found so far :)

Comment 11

[Jan de Mooij [:jandem]]

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 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Nothing bad found, tested on Mac and Windows 32-bit.

Comment 13

[Jan de Mooij [:jandem]]

And part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6a6fe7ccf6

Gary and Christian, thanks for the fuzzing help!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77141947c30f
https://hg.mozilla.org/mozilla-central/rev/0b6a6fe7ccf6