Closed Bug 680562 Opened 9 years ago Closed 8 years ago

Remove CHECK_INTERRUPT_HANDLER

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jorendorff, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER with jimb's new mechanism for enabling interrupts.
Depends on: onStep
Depends on: 650411
Awesome, these are way gross.
OS: Mac OS X → All
Hardware: x86 → All
Version: Other Branch → Trunk
I just noticed this is unblocked.
Attached patch rm (obsolete) — Splinter Review
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #640040 - Flags: review?(jimb)
OMG (In reply to Jason Orendorff [:jorendorff] from comment #0)
> Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER
> with jimb's new mechanism for enabling interrupts.

???? Oh, that. :(
Shouldn't there be a change to JS_SetInterrupt, JS_ClearInterrupt, to actually use the InterpreterFrames chain, as JSScript::initScriptCounts and JSScript::ensureHasDebugScript do?
Comment on attachment 640040 [details] [diff] [review]
rm

Review of attachment 640040 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ -917,5 @@
>  template<typename T>
>  class GenericInterruptEnabler : public InterpreterFrames::InterruptEnablerBase {
>    public:
>      GenericInterruptEnabler(T *variable, T value) : variable(variable), value(value) { }
> -    void enableInterrupts() const { *variable = value; }

"There's no need to restate the type name in member function names." Thanks!

::: js/src/jsinterp.h
@@ +241,5 @@
>      ~InterpreterFrames();
>  
>      /* If this js::Interpret frame is running |script|, enable interrupts. */
>      inline void enableInterruptsIfRunning(JSScript *script);
> +    inline void enableInterruptsUnconditionally() { enabler.enable(); }

This has no callers, but it seems like it needs to be used by JS_SetInterrupt.
(In reply to Jim Blandy :jimb from comment #5)

You're right, but it's weird, because I remember writing that code; I guess I lost it somehow...

> This has no callers, but it seems like it needs to be used by JS_SetInterrupt.

Yeah, that is what I added for JS_SetInterrupt :P
Attached patch rmSplinter Review
qref'd 'n everything

On a related note: has it every happened to you that you have some un-qref-d changes, then you push an empty patch for the try-server message, push to try, qpop, qdel the try patch, and then notice you've lost your changes?
Attachment #640040 - Attachment is obsolete: true
Attachment #640040 - Flags: review?(jimb)
Attachment #642818 - Flags: review?(jimb)
(In reply to Luke Wagner [:luke] from comment #8)
> On a related note: has it every happened to you that you have some un-qref-d
> changes, then you push an empty patch for the try-server message, push to
> try, qpop, qdel the try patch, and then notice you've lost your changes?

Well, if you have uncommitted changes and qnew (to add the try chooser text), then that qnew will gather up the uncommitted changes into the new patch. And then you pop and qdel that. Possible?
VC tools should never lose your work; qdel should totally support "undo". But I think it's right for qnew to gather up uncommitted changes.
(In reply to Jim Blandy :jimb from comment #9)
Yes, that's exactly what happens.  I've had to retrieve the patch from try server before :)
Comment on attachment 642818 [details] [diff] [review]
rm

Review of attachment 642818 [details] [diff] [review]:
-----------------------------------------------------------------

Okay --- that makes more sense!
Attachment #642818 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/8e406f71fadb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.