Bug 155741 – We should not disable inlining when the debugger is enabled

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 274674 [details]
WIP

We probably need to do something similar for stepping mode.

[Saam Barati]

(In reply to comment #1)
> Created attachment 274674 [details]
> WIP
> 
> We probably need to do something similar for stepping mode.

Actually I don't think we do. Setting the SteppingMode seems to target
all code blocks and jettison all optimized code blocks.

[Saam Barati]

Created attachment 274723 [details]
patch

[WebKit Commit Bot]

Comment on attachment 274723 [details]
patch

Clearing flags on attachment: 274723

Committed r198582: <http://trac.webkit.org/changeset/198582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 274723 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274723&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-451
> -            else if (m_hasDebuggerEnabled && operand == m_codeBlock->scopeRegister())
> -                flush(operand);

I think we still need to flush the scope register.

If A calls B, and both A and B are DFG-compiled, and B is not inlined, and I break in B, the debugger allows me to walk the stack and inspect A's local variables. But if the scope register is not flushed, I might crash when I try to inspect A, reading a garbage scope pointer from the stack.

[Joseph Pecoraro]

Comment on attachment 274723 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274723&action=review

> Source/JavaScriptCore/debugger/Debugger.cpp:268
> +        line += 1;
> +        column = column ? column + 1 : Breakpoint::unspecifiedColumn;

Would be nice to comment why these +1. It used to have a comment.

> Source/JavaScriptCore/debugger/Debugger.cpp:-266
> -    // Inspector breakpoint line and column values are zero-based but the executable
> -    // and CodeBlock line and column values are one-based.

This was the comment it used ot have.

[Saam Barati]

(In reply to comment #6)
> Comment on attachment 274723 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274723&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-451
> > -            else if (m_hasDebuggerEnabled && operand == m_codeBlock->scopeRegister())
> > -                flush(operand);
> 
> I think we still need to flush the scope register.
> 
> If A calls B, and both A and B are DFG-compiled, and B is not inlined, and I
> break in B, the debugger allows me to walk the stack and inspect A's local
> variables. But if the scope register is not flushed, I might crash when I
> try to inspect A, reading a garbage scope pointer from the stack.

Good call. I will bring these changes back.

[Saam Barati]

(In reply to comment #7)
> Comment on attachment 274723 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274723&action=review
> 
> > Source/JavaScriptCore/debugger/Debugger.cpp:268
> > +        line += 1;
> > +        column = column ? column + 1 : Breakpoint::unspecifiedColumn;
> 
> Would be nice to comment why these +1. It used to have a comment.
> 
> > Source/JavaScriptCore/debugger/Debugger.cpp:-266
> > -    // Inspector breakpoint line and column values are zero-based but the executable
> > -    // and CodeBlock line and column values are one-based.
> 
> This was the comment it used ot have.

Nice catch. I'll add those back.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155812

[Saam Barati]

Lets start by having a high fidelity profiling mode first. Then we can revisit this again.