TraceLogger: Disable when combining TraceLogger with an exception, ionmonkey and the debugger

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
When getting an exception getting from ionmonkey with inlined frames we do something special if the debugger is enabled. That is because debugger can read/adjust everything, I suppose. Allowing TraceLogger to continue and be in the correct state seems very hard. Also not sure if we would want to use the tracelogger together with the debugger. It seems that at that point you've given up all hope for speed.

As a result I think we can just forcefully disable TraceLogger. That way TraceLogger will have all correct information until the exception happens, where it will stop logging.

I think that is the best solution for now. And if we want to start supporting this we can always start implementing it when asked.
(Assignee)

Comment 1

2 years ago
Created attachment 8787264 [details] [diff] [review]
Patch
Assignee: nobody → hv1989
Attachment #8787264 - Flags: review?(bbouvier)
(Assignee)

Updated

2 years ago
Blocks: 1299813
Comment on attachment 8787264 [details] [diff] [review]
Patch

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

Makes sense.

::: js/src/jit/JitFrames.cpp
@@ +789,5 @@
>  
> +#ifdef JS_TRACE_LOGGING
> +            if (logger && cx->compartment()->isDebuggee() && logger->enabled()) {
> +                logger->disable(/* force = */ true);
> +                fprintf(stderr, "Forcefully disabled tracelogger, due to "

Care to use JitSpewer instead, to not provoke differential testing issues?
Attachment #8787264 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8787640 [details] [diff] [review]
TLOPTIONS=Error

Instead if JitSpewer, about about TLOPTIONS=Errors
That looks cleaner to me, since IONFLAGS/JitSpewer is quite specific to jits.
Attachment #8787640 - Flags: review?(bbouvier)
Comment on attachment 8787640 [details] [diff] [review]
TLOPTIONS=Error

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

Thanks! I realized late that JitSpewer was very tied to the Jits; what I meant is something like you've done, so this is perfect.
Attachment #8787640 - Flags: review?(bbouvier) → review+

Comment 5

2 years ago
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/693970250ff7
TraceLogger - Disable on throwing an exception in IonMonkey with Debugger enabled, r=bbouvier

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/693970250ff7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.