Closed Bug 1060936 Opened 5 years ago Closed 5 years ago

Directly call debugger hooks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #8481954 - Flags: review?(jorendorff)
Assignee: nobody → evilpies
Depends on: 800200
No longer depends on: 1031880
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)
Status: NEW → ASSIGNED
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)
Blocks: 1069694
Attached patch v2 (obsolete) — Splinter Review
Attachment #8481954 - Attachment is obsolete: true
Attachment #8493676 - Flags: review?(shu)
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)
Attached patch v3Splinter Review
Attachment #8493676 - Attachment is obsolete: true
Attachment #8498817 - Flags: review?(shu)
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+
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
https://hg.mozilla.org/mozilla-central/rev/2374287a24ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.