Closed
Bug 604504
Opened 14 years ago
Closed 14 years ago
Consolidate eval functionality into a single utility function, and use that directly from JSOP_EVAL
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(3 files, 2 obsolete files)
5.35 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
19.24 KB,
patch
|
jorendorff
:
review+
dvander
:
review+
|
Details | Diff | Splinter Review |
19.54 KB,
patch
|
Details | Diff | Splinter Review |
JSOP_EVAL is functionally identical to JSOP_CALL right now, except that obj_eval bytecode-inspects the most recent scripted function to see whether it was called via JSOP_EVAL or not. This is problematic in ES5, where eval called from native code must be treated as an indirect eval (even if there's no script around at all -- a case in which we currently, and incorrectly, throw an exception). Let's separate JSOP_EVAL's implementation into something that calls eval's core functionality directly, also indicating directness when doing so, and then we can make the eval function itself call that core function and indicate indirectness when doing so. This also improves readability of the code, and it helps pave the way toward implementation of bug 514568.
Assignee | ||
Comment 1•14 years ago
|
||
This just splits the eval opcode out -- it doesn't change how eval functionality is implemented beyond the opcode level.
Attachment #483301 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•14 years ago
|
||
This is where things actually get a little interesting. I move most of obj_eval into a js::EvalKernel function, then make the three eval implementations (interpreter, JM, eval function itself) all call that, with varying arguments depending on the context. Unfortunately this requires pushing a few checks upward a little and duplicating them (things like the CSP check, zero-args, two-args, non-string arg), but it's not too bad: I don't think we really want EvalKernel to have a function-like interface (one that doesn't just take a string of code to eval, but Value* plus length instead) that's more obscure and easier to use to avoid it.
Attachment #483311 -
Flags: review?(jorendorff)
Comment 3•14 years ago
|
||
There's too much copy and paste going on in part 2 for my taste. Here's a patch that ends up with 100 fewer lines (actually something like 130 since I also cut out some code that's totally unnecessary with the patch and refactored a few other things). Two more notes: In js/src/methodjit/Compiler.cpp: > BEGIN_CASE(JSOP_EVAL) >- jsop_eval(); >+ { >+ JaegerSpew(JSpew_Insns, " --- SCRIPTED EVAL --- \n"); >+ emitEval(GET_ARGC(PC)); >+ JaegerSpew(JSpew_Insns, " --- END SCRIPTED EVAL --- \n"); >+ } > END_CASE(JSOP_EVAL) ... >+void >+mjit::Compiler::emitEval(uint32 argc) >+{ >+ /* Check for interrupts on function call */ >+ interruptCheckHelper(); >+ >+ frame.syncAndKill(Registers(Registers::AvailRegs), Uses(argc + 2)); >+ prepareStubCall(Uses(argc + 2)); >+ masm.move(Imm32(argc), Registers::ArgReg1); >+ stubCall(stubs::Eval); >+ frame.popn(argc + 1); >+} dvander, could you take a look at this? I tried to compare it to what we do now (in inlineCallHelper) and got lost pretty quickly. And this is missing a test. The ES5 conformance bug this fixes is ludicrous, but it's there, right? I think it's something like this: var x = 1; var realEval = eval; function test() { var x = 2; var eval = Array.map; var a = eval(["x"], realEval); // indirect eval assertEq(a[0], 1); } test(); ...Or was that already fixed? I'm sure this test case is in a bug somewhere else, but I couldn't find it.
Comment 4•14 years ago
|
||
Comment on attachment 483301 [details] [diff] [review] 1 - Give JSOP_EVAL its own implementation, distinct from JSOP_CALL/JSOP_APPLY In BEGIN_CASE(JSOP_EVAL): >+ JS_ASSERT(vp[1].isObjectOrNull() || PrimitiveThisTest(newfun, vp[1])); Incidentally, with just part 1 applied, this assertion always fails, because vp[1] is always undefined. (The assertion gets removed in part 2, but it would be even better not to add it in the first place...) In jsobj.cpp: >-static inline JSScript ** >+namespace { >+ >+inline JSScript ** > EvalCacheHash(JSContext *cx, JSString *str) This makes it a little harder to set a breakpoint on the function (or call it, etc.) in GDB, so please don't do it. r=me on the other parts.
Attachment #483301 -
Flags: review?(jorendorff) → review+
Updated•14 years ago
|
Attachment #483311 -
Flags: review?(jorendorff) → review-
Comment 5•14 years ago
|
||
What is the point of namespace { ... } that I see being introduced (only by Waldo, I think)? Besides hurting gdb'ability it just adds boilerplate. Is anyone seriously worried about EvalCacheHash or whatever colliding with a standard header file non-namespaced-global name? Apart from windows.h evil CONST, etc., usurpations, which are IIRC macros so there is no namespace solution, this is not a real-world issue to bloat our code over-defensively against. /be
Assignee | ||
Comment 6•14 years ago
|
||
It's just static-scoping by block, rather than by bloating every declaration with the same information. (Yes, I said bloating -- if you can engage in hyperbole about just how "bad" this is, so can I. :-P) And "b jsobj.cpp:eval" does not seem hard to type to me. As for "only by me", MXR suggests it is becoming ubiquitous throughout the Mozilla codebase: http://mxr.mozilla.org/mozilla-central/search?string=namespace+\{®exp=on&find=&findi=&filter=^[^\0]*$&hitlimit=&tree=mozilla-central So even if I didn't use it (and I'm not the only one even in js/src, I see Luke has used it in several files), you're still going to have to get used to it. :-P Comments on the counterproposal coming shortly, after I type them up.
Comment 7•14 years ago
|
||
Forgot to mention: If you want an internal API that doesn't require a vp, factor the counterproposal's EvalKernel into two parts, of which the first part uses vp, and the second part has arguments like your original EvalKernel. Then you'll have the API you wanted without duplicating code.
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 484455 [details] [diff] [review] counterproposal I still prefer the interface to the eval functionality being a pass-in-a-string interface, but I guess it doesn't matter all that much, and this is indeed less code. The CSP bugfix here (that eval called without arguments or with a non-string first argument wasn't being blocked if the security policy said so) needs to be tested, in addition to the indirect eval bit you mention in previous comments. >diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp > bool >+DirectEval(JSContext *cx, JSFunction *evalfun, uint32 argc, Value *vp) >+{ >+ JS_ASSERT(vp == cx->regs->sp - argc - 2); >+ JS_ASSERT(vp[0].isObject()); >+ JS_ASSERT(vp[0].toObject().isFunction()); >+ JS_ASSERT(vp[0].toObject().getFunctionPrivate() == evalfun); >+ JS_ASSERT(IsBuiltinEvalFunction(evalfun)); >+ >+ AutoFunctionCallProbe callProbe(cx, evalfun); >+ >+ JSStackFrame *caller = cx->fp(); >+ JS_ASSERT(caller->isScriptFrame()); >+ JSObject *scopeChain = js_GetScopeChainFast(cx, caller, JSOP_EVAL, JSOP_EVAL_LENGTH + JSOP_LINENO_LENGTH); >+ if (!scopeChain || !EvalKernel(cx, argc, vp, EVAL_DIRECT, caller, scopeChain)) >+ return false; >+ cx->regs->sp = vp + 1; >+ return true; >+} I am not a fan of implicitly passing arguments (the stack frame, here). It seems better to be explicit, even if that means you add an assertion that the provided frame is cx->fp(). >diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h >+extern JS_REQUIRES_STACK bool >+DirectEval(JSContext *cx, JSFunction *evalfun, uint32 argc, Value *vp); Needs a brief descriptive comment. >diff --git a/js/src/jsobj.h b/js/src/jsobj.h >+enum EvalType { EVAL_INDIRECT, EVAL_DIRECT }; These read better as "indirect eval" and "direct eval", so INDIRECT_EVAL and DIRECT_EVAL. (But what in particular was wrong with the original names?) >diff --git a/js/src/methodjit/InvokeHelpers.cpp b/js/src/methodjit/InvokeHelpers.cpp >+void JS_FASTCALL >+stubs::Eval(VMFrame &f, uint32 argc) >+{ >+ Value *vp = f.regs.sp - (argc + 2); >+ >+ JSObject *callee; >+ JSFunction *fun; >+ >+ if (!IsFunctionObject(*vp, &callee) || >+ !IsBuiltinEvalFunction((fun = callee->getFunctionPrivate()))) >+ { >+ if (!ComputeThisFromVpInPlace(f.cx, vp) || >+ !Invoke(f.cx, InvokeArgsAlreadyOnTheStack(vp, argc), 0)) >+ { >+ THROW(); >+ } >+ return; >+ } >+ >+ JS_ASSERT(f.regs.fp == f.cx->fp()); >+ if (!DirectEval(f.cx, fun, argc, vp)) >+ THROW(); >+ return; >+} It doesn't appear to me that the unnecessary trailing |return;| is idiomatic.
Comment 9•14 years ago
|
||
(In reply to comment #6) > It's just static-scoping by block, rather than by bloating every declaration > with the same information. (Yes, I said bloating -- if you can engage in > hyperbole about just how "bad" this is, so can I. :-P) Settle down. Bloat is bloat, every line counts. What is the point of the extra lines and keystrokes, exactly? Purity points in an afterlife do not matter here. > And "b jsobj.cpp:eval" does not seem hard to type to me. I'm with Jason, keystrokes matter. This shows all over the place (one example: Unix commands vs. VMS). > As for "only by me", MXR suggests it is becoming ubiquitous throughout the > Mozilla codebase: Is it worth getting agreement and putting it in the style guide, instead of getting defensive in a bug and not addressing Jason's point? I think so. > So even if I didn't use it (and I'm not the only one even in js/src, I see Luke > has used it in several files), you're still going to have to get used to it. > :-P No, you're going to have to justify it a bit better and docuemnt it in the style guide if you persuade your peers. /be
Comment 10•14 years ago
|
||
And when I write peers, I do not mean to imply that everyone's equal or anything. And I'm not just talking about module owner vs. peers here. /be
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9) > Bloat is bloat, every line counts. What is the point of the extra lines and > keystrokes, exactly? Purity points in an afterlife do not matter here. My point (which I really should have made without committing the same error, apologies for doing so) is that describing something as "bloat", when such description is debatable, is unhelpfully incendiary (case in point: my response was as bad). As for extra lines/keystrokes, there are tradeoffs. Using an anonymous namespace adds four lines of code. But it also permits removal of each enclosed method's "static" annotation, a net reduction in characters given even a few enclosed methods. And moreover, this elimination produces a slight readability win for each method by not requiring the reader to gloss over "static" before reading method return types. > > And "b jsobj.cpp:eval" does not seem hard to type to me. > > I'm with Jason, keystrokes matter. This shows all over the place (one example: > Unix commands vs. VMS). This can be taken too far: creat and O_CREAT were still mistakes. I'm not saying this is the case here, but I do think we place too much value on concision over readability and understandability. > Is it worth getting agreement and putting it in the style guide, instead of > getting defensive in a bug and not addressing Jason's point? I think so. Which point did I not address? I argued that the "hardship" is not so onerous as he suggests, but yet in the end I agreed to acquiesce to his request. (For what it's worth, I once thought as he did. In particular, I didn't even know you could set a breakpoint on an anonymous-namespace method except by line number. But then I learned about the file:method debugging convention, and immediately the hardship was not markedly worse than it had been. So a part of me wondered if, perhaps, he too was unaware of that method of naming anonymous-namespace functions, and therefore I obliquely pointed it out.) > No, you're going to have to justify it a bit better and docuemnt it in the > style guide if you persuade your peers. That wasn't an intentional, deliberate attempt to justify it to peers. I was simply stating that, regardless what SpiderMonkey might choose based on its own usability considerations, SpiderMonkey hackers will have to deal with anonymous namespaces in Mozilla code. If we truly think them that hard to deal with when debugging, and I continue to think they pose no problem, we should argue for against them in all of Mozilla as well. Revised patch with tests coming in a few minutes...
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #483311 -
Attachment is obsolete: true
Attachment #484455 -
Attachment is obsolete: true
Attachment #485208 -
Flags: review?(jorendorff)
Assignee | ||
Comment 13•14 years ago
|
||
I landed part 1 in <http://hg.mozilla.org/tracemonkey/rev/40d0afa071d6>. Part 2, of course, remains unlanded.
Comment 14•14 years ago
|
||
(In reply to comment #11) > (In reply to comment #9) > > Bloat is bloat, every line counts. What is the point of the extra lines and > > keystrokes, exactly? Purity points in an afterlife do not matter here. > > My point (which I really should have made without committing the same error, > apologies for doing so) is that describing something as "bloat", when such > description is debatable, is unhelpfully incendiary (case in point: my response > was as bad). Apologies from my side: I did not mean to start fires. Bloat in this case meant any unnecessary source lines, where the "unnecessary" part needs debate. I don't have a dog in this fight apart from visceral sympathy with jorendorff's complaint about taxing debugger-invokers. I've felt that pain. OTOH such things as JS in the future (Harmony simple modules) may have such "static file scope" declarative boilerplate wrapping, and it has a function. It is also minor bloat compared to, e.g., the arrant code duplication between the interpreter and the methodjit's slow-call stubs. > As for extra lines/keystrokes, there are tradeoffs. Using an anonymous > namespace adds four lines of code. But it also permits removal of each > enclosed method's "static" annotation, a net reduction in characters given even > a few enclosed methods. Good point, although apples to oranges w.r.t. the debugger-usability point. > And moreover, this elimination produces a slight > readability win for each method by not requiring the reader to gloss over > "static" before reading method return types. I glossed long ago, but again you have an orange-point to Jason's apple-point. > > I'm with Jason, keystrokes matter. This shows all over the place (one example: > > Unix commands vs. VMS). > > This can be taken too far: creat and O_CREAT were still mistakes. Even ken said so, you're right. > I'm not > saying this is the case here, but I do think we place too much value on > concision over readability and understandability. I still think you crossed a line in js.msg and hope you take this point. > > Is it worth getting agreement and putting it in the style guide, instead of > > getting defensive in a bug and not addressing Jason's point? I think so. > > Which point did I not address? I argued that the "hardship" is not so onerous > as he suggests, but yet in the end I agreed to acquiesce to his request. You still aren't acknowledging the debugger-usability point. Dyanmic frequency does not equal static (writer or even code-reader) frequency. > That wasn't an intentional, deliberate attempt to justify it to peers. But why not? I really hope you do make case (Luke too if he agrees) and change the style guide. Again, I'm not betting on either outcome. I can live with (and grow to like) namespace { ... } wrappings to save some statics, even at the cost of hurting debugger-driving fingers. What I expect as module owner is this: no style wars, even where the conflict is avoided in favor of patch war by attrition; consensus forged explicitly, even if someone disagrees (once, well-recorded; e.g. jorendorff dissenting from the win of mutable reference types meaning non-null types). /be
Comment 15•14 years ago
|
||
> But why not? I really hope you do make case (Luke too if he agrees) and change
"make the case", of course.
/be
Comment 16•14 years ago
|
||
I often find it useful, when reading a function, to know if it is a TU-local helper or a SM-wide procedure. 'static' and unnamed namespaces both give this information. However, 'static' always says it where I land with vim-grep or ctags: on the decl. With unnamed namespaces, the "namespace {" may be lexically farther away. Granted, vim gives you [{ to hop up to enclosing curlies, but that's still more work. Thus, I prefer 'static' on the fun decl. I don't see the character delta as meaningful: when reading, our eyes are already trained to skip over the static type goo when we don't care (supported by the style rule of having a lone '{' on its own line to divide goo from the body). Although I'd have to check D&E to see if it says differently, I believe anonymous namespaces were an answer to "what if I want member functions or static member variables to have internal linkage?" However, the C++ (98 and 1x) spec seems to think more highly of them as it actually deprecates static since "the unnamed-namespace provides a superior alternative". I could be missing a detail, but I can't find any meaningful differences -- other than unnamed namespaces working on classes -- so I'm just going to assume that this is an attempt to collapse duplicate functionality (and remove a meaning of 'static'). I can't imagine that static-the-function-storage-class-specifier will be removed any time soon ;)
Assignee | ||
Comment 17•14 years ago
|
||
I think debugging concerns are overblown compared to reading concerns -- a lot more of the latter happens than the former, I believe, more than enough to outweigh the static-versus-external query impairment. Moreover, the local-versus-extern question is most relevant to the least-used functions where we end up less often when debugging. The common case is that we all recognize symbols like js_SetPropertyHelper (SetPropertyHelper if it is eventually renamed) are widely used and not file-local, so the concern about anonymous namespacing would come up less often than "each time I want to break in such a function". (It would be a minor barrier to newcomers, but only a minor one, and moreso than many others.) But since other consensus seems to be against, I'll deal with that. One last bit (of course the modified patch-review still stands ready when jorendorff has the time amongst his other pressing demands to get to it): (In reply to comment #14) > I still think you crossed a line in js.msg and hope you take this point. Is this a reference to something outside this bug? I'm having trouble precisely determining the referent and would appreciate a reminder.
Assignee | ||
Comment 18•14 years ago
|
||
(In reply to comment #17) > (It would be a minor barrier to newcomers, but only a minor one, and moreso > than many others.) Er, "more minor". This commenting thing, I am not good with it today. Filed bug 607148 on removing unnamed namespace use in js/src/.
Comment 19•14 years ago
|
||
I meant overlong names and ragged-middle indentation re: js.msg. There's an old hacker-culture norm: make your patch blend in. It is fuzzy of course. But it's like working irl. If you help a friend remodel his house, and the plans are underspecified (all plans are), you don't start doing your favorite Victorian Fishscale when his remodel is clearly Bauhaus modern. Ok, those are too far apart; maybe Queen Anne vs. Italianate Victorian works better -- but you get the idea. /be
Comment 20•14 years ago
|
||
(In reply to comment #16) > I often find it useful, when reading a function, to know if it is a TU-local > helper or a SM-wide procedure. Same here. Forget the debugger; the real issue is, is this information just noise, or is it important enough to retain it at the top of such functions? I think the latter.
Comment 21•14 years ago
|
||
Comment on attachment 485208 [details] [diff] [review] 2 - EvalKernel and friends as jorendorff proposes Looks great.
Attachment #485208 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 22•14 years ago
|
||
Comment on attachment 485208 [details] [diff] [review] 2 - EvalKernel and friends as jorendorff proposes ...just for the mjit::Compiler::emitEval bit.
Attachment #485208 -
Flags: review?(dvander)
Comment on attachment 485208 [details] [diff] [review] 2 - EvalKernel and friends as jorendorff proposes > void >-mjit::Compiler::jsop_eval() >+mjit::Compiler::emitEval(uint32 argc) > { >- JaegerSpew(JSpew_Insns, " --- SCRIPTED CALL --- \n"); >- inlineCallHelper(GET_ARGC(PC), false); >- JaegerSpew(JSpew_Insns, " --- END SCRIPTED CALL --- \n"); >+ /* Check for interrupts on function call */ >+ interruptCheckHelper(); >+ >+ frame.syncAndKill(Registers(Registers::AvailRegs), Uses(argc + 2)); >+ prepareStubCall(Uses(argc + 2)); >+ masm.move(Imm32(argc), Registers::ArgReg1); >+ stubCall(stubs::Eval); >+ frame.popn(argc + 1); The last line should be, frame.popn(argc + 2); frame.pushSynced(); Otherwise the return value of eval() might not be communicated properly, the bottom of the stack could be tracked as a constant or as having a known type. r=me with that
Attachment #485208 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 24•14 years ago
|
||
Trying to include a testcase that will fail without his changes, not succeeding, posting update so he can try to construct one, as my/our attempts are failures so far...
Assignee | ||
Comment 25•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/6abb9e45a79a ...for part 2, so this is now fixed in TM.
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 26•14 years ago
|
||
Hm, I didn't intend to remove the cross-compartment check when doing this! I'm adding the code back to do that shortly, with a test, as soon as I write it.
Assignee | ||
Comment 27•14 years ago
|
||
...or never mind -- such calls end up going through wrappers, and those wrappers certainly won't have eval as their native function. Or at least that's what it appears like for this testcase in a debugger: var s = evalcx(""); function test() { var eval = s.eval; return eval("'test' in this"); } test();
Comment 28•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6abb9e45a79a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•