Bug 127269 – A few utility functions for setting/clearing the op_debug hasBreakpointFlag.

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

[Mark Lam]

The bytecode level breakpoint debugger will have to set and clear breakpoints by setting/clearing the hasBreakpointFlag operand in the op_debug bytecode of a CodeBlock.  In addition, as an optimization, there’s also a need to for a utility function clear all breakpoints in a CodeBlock that may have been set.  This is useful when a debugger detaches, and breakpoints need to be cleared in bulk.  This patch will provide these utility functions in anticipation of upcoming support for bytecode level breakpoints.  These functions are not currently in use.

[Mark Lam]

Created attachment 221628 [details]
the patch.

[Mark Lam]

FYI, the efl-wk2 bot failure is due to an internal compiler error on the bot.  Hence, it is not due to the patch.

[Geoffrey Garen]

Comment on attachment 221628 [details]
the patch.

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

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:406
> +void UnlinkedCodeBlock::clearAllBreakpointsIn(RefCountedArray<Instruction>& instructions)
> +{
> +    LineColumnInfoList& list = opDebugLineColumnInfoList();
> +    for (LineColumnInfoList::iterator it = list.begin(); it != list.end(); ++it) {
> +        static const int hasBreakpointFlagOffset = 2; 
> +        unsigned offset = it->instructionOffset;
> +        ASSERT(m_vm->interpreter->getOpcodeID(instructions[offset].u.opcode) == op_debug);
> +        instructions[offset + hasBreakpointFlagOffset] = false;
> +    }
> +}

This doesn't look right. The unlinked CodeBlock should never contain breakpoints, or metadata about whether they're set. The unlinked CodeBlock is cached and shared among webpages. Putting breakpoint metadata in it would cause debugging in one webpage to set breakpoints in another, and it would cause breakpoints to persist across page loads.

[Mark Lam]

(In reply to comment #3)
> (From update of attachment 221628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221628&action=review
> 
> > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:406
> > +void UnlinkedCodeBlock::clearAllBreakpointsIn(RefCountedArray<Instruction>& instructions)
> > +{
> > +    LineColumnInfoList& list = opDebugLineColumnInfoList();
> > +    for (LineColumnInfoList::iterator it = list.begin(); it != list.end(); ++it) {
> > +        static const int hasBreakpointFlagOffset = 2; 
> > +        unsigned offset = it->instructionOffset;
> > +        ASSERT(m_vm->interpreter->getOpcodeID(instructions[offset].u.opcode) == op_debug);
> > +        instructions[offset + hasBreakpointFlagOffset] = false;
> > +    }
> > +}
> 
> This doesn't look right. The unlinked CodeBlock should never contain breakpoints, or metadata about whether they're set. The unlinked CodeBlock is cached and shared among webpages. Putting breakpoint metadata in it would cause debugging in one webpage to set breakpoints in another, and it would cause breakpoints to persist across page loads.

I’m not touching the UnlinkedCodeBlock bytecode here.  The bytecode instructions array is provided by the CodeBlock and passed in.  What the UnlinkedCodeBlock has is a map of where all the op_debug bytecodes are so that we can get to them quickly.  I opted to have the UnlinkedCodeBlock provided this utility function that the CodeBlock can use to clear its breakpoints because I wanted to keep that list private.

The alternative is to make opDebugLineColumnInfoList() public and have CodeBlock walk it to do the breakpoint clearing.

[Mark Lam]

We have a change of plans.  Instead of going with bytecode level breakpoints, we're going with CodeBlock level breakpoints.  This patch is no longer relevant.