636296 - simplify activation object creating/putting logic

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Working on bug 635811 and bug 634542, I saw a couple ways we could simplify the logic surrounding activation object creating/putting.  The patches got too big for FF4, so I had to make quick 'n dirty fixes instead, but I'd still like to finish and land the patches post 4.0.  In particular:
 - putting call objects when we pop the frame, not in the ScriptEpilogue (we create the call object when we push the frame, not in ScriptPrologue).
 - s/fp->hasCallObj/fp->hasOwnCallObj/ so that "if (fp->hasOwnCallObj()) PutCallObject(fp->callObj())" is always correct (as opposed to now with the whole non-strict eval situation)

Comment 1

[Luke Wagner [:luke]]

Attachment: part 1: tidy up JSStackFrame

Comment 2

[Luke Wagner [:luke]]

Attachment: part 2: tidy up ScriptPrologue/Epilogue

Comment 3

[Luke Wagner [:luke]]

Attachment: part 3: simplify meaning of hasCallObj

Comment 4

[Luke Wagner [:luke]]

Attachment: part 4: put activation objects when pop frame, not in ScriptEpilogue

Comment 5

[Luke Wagner [:luke]]

Passes trace tests.  I won't bother anyone for reviews until after FF4.0, but I thought it would be good to leave these here.

Comment 6

[Luke Wagner [:luke]]

Attachment: part 1: tidy up JSStackFrame

Posting rebased patches and getting review.  The first two are really simple.  This patch just fixed a naming complaint Steve Fink had on infomonkey.

Comment 7

[Luke Wagner [:luke]]

Attachment: part 2: tidy up ScriptPrologue/Epilogue

This splits prologue/epilogue into the parts that deal with debug stuff and the parts that deal with core semantics.

Comment 8

[Luke Wagner [:luke]]

Attachment: part 3: simplify meaning of hasCallObj

This patch gives 'hasCallObj' the meaning: if a frame 'hasCallObj', then the call obj should be put when the frame is popped (thereby eliminating the eval-frame corner case).

Comment 9

[Luke Wagner [:luke]]

Attachment: part 4: put activation objects when the frame is popped, not ScriptEpilogue

Comment 10

[Luke Wagner [:luke]]

Oops, need to fix InjectJaegerReturn; will make a part 5 patch.

Comment 11

[Luke Wagner [:luke]]

Attachment: part 4: put activation objects when the frame is popped, not ScriptEpilogue

Actually, InjectJaegerReturn can just die; its equivalent to the forceReturnTrampoline, which works.

Comment 12

[Luke Wagner [:luke]]

Attachment: part 3: simplify meaning of hasCallObj

Fix bug (strict eval's call obj not getting put in initialVarObj).

With that, green on try.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 519447 [details] [diff] [review]
part 4: put activation objects when the frame is popped, not ScriptEpilogue

Nice, this is much simpler. One nit, markActivationObjectsAsHavingBeenPut is kind of verbose. How about markActivationObjectsAsPut?

Comment 14

[Luke Wagner [:luke]]

Oh, hah, yeah, that is much preferable.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 519527 [details] [diff] [review]
part 3: simplify meaning of hasCallObj

You have one "NB." which more typically is "NB:".  Other than that, much awesomeness to behold.

Comment 16

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/d7c35d34c202
http://hg.mozilla.org/tracemonkey/rev/9484a9805efa
http://hg.mozilla.org/tracemonkey/rev/dbb123c798c8
http://hg.mozilla.org/tracemonkey/rev/d839300746c3

Comment 17

[Makoto Kato [:m_kato]]

Part 4 causes crash on Win64 and Solaris x86.  These platforms have to consider stack adjustment.

Comment 18

[Makoto Kato [:m_kato]]

Attachment: crash fix for Win64

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 521451 [details] [diff] [review]
crash fix for Win64

Could you instead factor out this hack into JaegerCompartment so that it gets fixed in one place instead of 2+ ?

Comment 20

[Luke Wagner [:luke]]

Oops, I meant to add: thank you for debugging and fixing :)

Comment 21

[Makoto Kato [:m_kato]]

(In reply to comment #19)
> Comment on attachment 521451 [details] [diff] [review]
> crash fix for Win64
> 
> Could you instead factor out this hack into JaegerCompartment so that it gets
> fixed in one place instead of 2+ ?

PushActiveVMFrame set return function to JaegerTrampolineReturn. JaegerTrampolineReturn adjusts stack for non-FASTCALL ABI and Win64 ABIT.   This point needs restore stack.

Comment 22

[Luke Wagner [:luke]]

I was referring to the duplication in stubs::Debugger and stubs::Trap.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to comment #22)
> I was referring to the duplication in stubs::Debugger and stubs::Trap.

It is necessary to add stack adjustment excepting to js_InternalThrow().

After my fix, it passes test even if on Win64 build.

Comment 24

[Luke Wagner [:luke]]

Attachment: factored version of Makoto Kato's patch

I'm sorry, I was not communicating my intentions clearly.  Here is something like what I was asking for.

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/d7c35d34c202
http://hg.mozilla.org/mozilla-central/rev/9484a9805efa
http://hg.mozilla.org/mozilla-central/rev/dbb123c798c8
http://hg.mozilla.org/mozilla-central/rev/d839300746c3

Comment 26

[Luke Wagner [:luke]]

Landed attachment 522395 [details] [diff] [review]:
http://hg.mozilla.org/tracemonkey/rev/2d641bd67adf

Comment 28

[Cameron Kaiser [:spectre]]

I'm tracking a TenFourFox bug that seems to have been triggered by part 4. In our bug, the browser hiccoughs (and sometimes crashes) trying to malloc a ridiculously large amount of memory. Analysis shows that the actual failure is in NewArguments() being passed a huge argc, which multiplied by sizeof(Value) yields a huge number, and the argc is coming from args.nactual, which appears to be getting much less frequently updated as a result of that part. Our wallpaper patch is this very simple kludge,

diff --git a/js/src/jsinterpinlines.h b/js/src/jsinterpinlines.h
--- a/js/src/jsinterpinlines.h
+++ b/js/src/jsinterpinlines.h
@@ -393,17 +393,23 @@ JSStackFrame::varobj(JSContext *cx) cons
     return isFunctionFrame() ? callObj() : cx->activeSegment()->getInitialVarObj();
 }
 
 inline uintN
 JSStackFrame::numActualArgs() const
 {
     JS_ASSERT(hasArgs());
     if (JS_UNLIKELY(flags_ & (JSFRAME_OVERFLOW_ARGS | JSFRAME_UNDERFLOW_ARGS)))
+#if(0)
         return hasArgsObj() ? argsObj().getArgsInitialLength() : args.nactual;
+#else
+        return hasArgsObj() ? argsObj().getArgsInitialLength() :
+            (args.nactual < 1024*1024) ? args.nactual :
+            numFormalArgs();
+#endif
     return numFormalArgs();
 }
 
 inline js::Value *
 JSStackFrame::actualArgs() const
 {
     JS_ASSERT(hasArgs());
     js::Value *argv = formalArgs();

which repairs the problem. But I'm trying to figure out where it came from in the first place. Is this the right thing to do? The dynamics may be different for us because we're pure tracejit and have no methodjit yet.

4.0 didn't do this.

Comment 29

[Luke Wagner [:luke]]

Cameron: is there a reproducible test-case for this hiccough/crash?  (Also, if its not too much trouble, could you file a new bug instead of extending this one and CC me?)

Comment 30

[Cameron Kaiser [:spectre]]

Done (bug 663789). Thanks, Luke!