Directly call debugger hooks

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8481954 [details] [diff] [review]
v1
Attachment #8481954 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Assignee: nobody → evilpies
(Assignee)

Updated

4 years ago
Depends on: 800200
No longer depends on: 1031880
(Assignee)

Comment 1

4 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

4 years ago
Status: NEW → ASSIGNED

Comment 2

4 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)

Updated

4 years ago
Blocks: 1069694
(Assignee)

Comment 3

4 years ago
Created attachment 8493676 [details] [diff] [review]
v2
Attachment #8481954 - Attachment is obsolete: true
Attachment #8493676 - Flags: review?(shu)

Comment 4

4 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

4 years ago
Created attachment 8498817 [details] [diff] [review]
v3
Attachment #8493676 - Attachment is obsolete: true
Attachment #8498817 - Flags: review?(shu)

Comment 6

4 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+
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.