Closed
Bug 1060936
Opened 10 years ago
Closed 10 years ago
Directly call debugger hooks
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file, 2 obsolete files)
25.74 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8481954 -
Flags: review?(jorendorff)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Comment on attachment 8481954 [details] [diff] [review] v1 Shu you have been working on similar stuff, so I am moving the review to you.
Attachment #8481954 -
Flags: review?(jorendorff) → review?(shu)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
Comment on attachment 8481954 [details] [diff] [review] v1 Review of attachment 8481954 [details] [diff] [review]: ----------------------------------------------------------------- I don't like the inlining of setting the return value on the frame and setting/clearing the exception in the various callsites; it has more possible points of bugs than the current setup. DebugScriptEpilogue should go regardless though, that looks useless and doesn't return a trap status. If your goal is to remove the OldDebugAPI stuff, what do you think of refactoring the "what should we do on this frame according to this trap status" code into a static helper in Debugger? D::onEnterFrame and D::onExceptionUnwind can call that helper directly. ::: js/src/jit/IonFrames.cpp @@ +532,2 @@ > default: > MOZ_CRASH("Invalid trap status"); Since you changed the crash message to be more descriptive elsewhere in the patch, could you do it here as well? e.g., bad Debugger::onExceptionUnwind status. ::: js/src/vm/Interpreter.cpp @@ +1013,2 @@ > default: > MOZ_CRASH("Invalid trap status"); Ditto on this message. ::: js/src/vm/Interpreter.h @@ -72,5 @@ > - * pending exception. > - * > - * - JSTRAP_RETURN: Return from |frame|. DebugExceptionUnwind has cleared > - * |cx|'s pending exception and set |frame|'s return value. > - */ These are nice comments, it'd be a shame to delete them. If you take my suggestion about the "what should be done on this frame for this trap status" helper, you can then move these comments over to the various Debugger:: static methods.
Attachment #8481954 -
Flags: review?(shu)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8481954 -
Attachment is obsolete: true
Attachment #8493676 -
Flags: review?(shu)
Comment 4•10 years ago
|
||
Comment on attachment 8493676 [details] [diff] [review] v2 Review of attachment 8493676 [details] [diff] [review]: ----------------------------------------------------------------- Given nbp's recent changes, s/JS_ASSERT/MOZ_ASSERT Sorry for the churn; the sum of all these small changes isn't trivial, so I'd like to see a v3. Thanks for the cleanup again! ::: js/src/jit/VMFunctions.cpp @@ +785,5 @@ > case JSTRAP_ERROR: > return false; > > default: > + MOZ_CRASH("bar Debugger::onEnterFrame status"); s/bar/bad ::: js/src/vm/Debugger.cpp @@ +682,5 @@ > + break; > + > + default: > + MOZ_CRASH("Invalid trap status"); > + } While you're here, could you change the order of the cases to CONTINUE, THROW, ERROR, RETURN? ::: js/src/vm/Debugger.h @@ +466,5 @@ > + * exception. > + * > + * - JSTRAP_RETURN: Return from the new frame immediately. ScriptDebugPrologue > + * has set |frame|'s return value appropriately. > + */ s/ScriptDebugPrologue/onEnterFrame @@ +467,5 @@ > + * > + * - JSTRAP_RETURN: Return from the new frame immediately. ScriptDebugPrologue > + * has set |frame|'s return value appropriately. > + */ > + static inline JSTrapStatus onEnterFrame(JSContext *cx, AbstractFramePtr frame); Nit: newline. @@ +480,5 @@ > + * This function may be called twice for the same outgoing frame; only the > + * first call has any effect. (Permitting double calls simplifies some > + * cases where an onPop handler's resumption value changes a return to a > + * throw, or vice versa: we can redirect to a complete copy of the > + * alternative path, containing its own call to ScriptDebugEpilogue.) s/ScriptDebugEpilogue/onLeaveFrame @@ +486,1 @@ > static inline bool onLeaveFrame(JSContext *cx, AbstractFramePtr frame, bool ok); Nit: newline here and below. @@ +500,5 @@ > + * > + * - JSTRAP_RETURN: Return from |frame|. onExceptionUnwind has cleared > + * |cx|'s pending exception and set |frame|'s return value. > + */ > + static inline JSTrapStatus onExceptionUnwind(JSContext *cx, AbstractFramePtr frame); nit: extra space before JSTrapStatus and newline. ::: js/src/vm/Interpreter.cpp @@ +1028,5 @@ > > again: > if (cx->isExceptionPending()) { > /* Call debugger throw hooks. */ > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) { No need to check debugMode here, since all the Debugger:onFoo methods already check them. If you believe in the branch predictor hints, feel free to move the MOZ_UNLIKELY over into the various Debugger::onFoo methods. @@ +1034,1 @@ > switch (status) { Nit: change the "Invalid trap status" message in this switch to be more descriptive. @@ +1508,5 @@ > { > goto error; > } > } > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) { Ditto on no need to check twice. @@ +1790,5 @@ > TraceLogStopEvent(logger); > // Stop the script. (Again no details about which script exactly.) > TraceLogStopEvent(logger); > > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) Ditto on no need to check twice. @@ +2632,5 @@ > TraceLogStartEvent(logger, TraceLogger::Interpreter); > > if (!REGS.fp()->prologue(cx)) > goto error; > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) { Ditto on no need to check twice. @@ +3492,5 @@ > > MOZ_CRASH("Invalid HandleError continuation"); > > exit: > if (MOZ_UNLIKELY(cx->compartment()->debugMode())) Ditto on no need to check twice.
Attachment #8493676 -
Flags: review?(shu)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8493676 -
Attachment is obsolete: true
Attachment #8498817 -
Flags: review?(shu)
Comment 6•10 years ago
|
||
Comment on attachment 8498817 [details] [diff] [review] v3 Review of attachment 8498817 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: js/src/vm/Debugger-inl.h @@ +13,5 @@ > > inline bool > js::Debugger::onLeaveFrame(JSContext *cx, AbstractFramePtr frame, bool ok) > { > + JS_ASSERT_IF(frame.isInterpreterFrame(), frame.asInterpreterFrame() == cx->interpreterFrame()); MOZ_ASSERT_IF
Attachment #8498817 -
Flags: review?(shu) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d484a770816
Comment 8•10 years ago
|
||
sorry has to back this out since it was breaking non-unified builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=49533955&tree=Mozilla-Inbound
Assignee | ||
Comment 9•10 years ago
|
||
I think I fixed it. https://hg.mozilla.org/integration/mozilla-inbound/rev/2374287a24ab
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2374287a24ab
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•