In anticipation of bytecode level breakpoints, we need to register CodeBlocks and Executables with the Debugger. This is because the Debugger potentially needs to enable / disable breakpoints in the op_debug bytecode of those CodeBlocks. In order to do this, the Debugger needs be made aware of CodeBlocks it is working with, and this is accomplished via a registration process. The Debugger also needs to group the registered CodeBlocks by their Executables because it will need this information later for de-opting DFG CodeBlocks when we add DFG support to the Debugger. The Debugger will keep track of these CodeBlock and Executable registrations using a new ExecutableRegistrar class. This patch will add this infrastructure to do the registration and unregistration. but will not apply the information towards implementing breakpoints yet.
Created attachment 221636 [details] the patch.
Attachment 221636 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:68: get_less is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:69: set_less is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:70: get_greater is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:71: set_greater is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:73: get_balance_factor is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:74: set_balance_factor is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:76: compare_key_key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:77: compare_key_node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/debugger/DebuggerExecutableRegistrar.h:78: compare_node_node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/ChangeLog:121: Line contains tab character. [whitespace/tab] [5] Total errors found: 10 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Why can't the debugger just use the list of CodeBlocks and Executables already kept by the garbage collector?
(In reply to comment #3) > Why can't the debugger just use the list of CodeBlocks and Executables already kept by the garbage collector? Because the Debugger requires that the Executable info be sorted in a specific order (specified in DebuggerExecutableRegistrar.h) in order to make breakpoint searches expedient. Otherwise, every time we enable / disable a new breakpoint in the debugger, we’ll need to iterate through all CodeBlocks to check if the breakpoint falls in them. We could assume that the number of CodeBlocks in the VM is small enough that we don’t care about this performance difference. Or we could sort the garbage collector’s list, but that would mean impacting the perf of non-debugger code for debugger functionality that is not normally used.
(In reply to comment #4) > (In reply to comment #3) > > Why can't the debugger just use the list of CodeBlocks and Executables already kept by the garbage collector? > > Because the Debugger requires that the Executable info be sorted in a specific order (specified in DebuggerExecutableRegistrar.h) in order to make breakpoint searches expedient. Otherwise, every time we enable / disable a new breakpoint in the debugger, we’ll need to iterate through all CodeBlocks to check if the breakpoint falls in them. > > We could assume that the number of CodeBlocks in the VM is small enough that we don’t care about this performance difference. > > Or we could sort the garbage collector’s list, but that would mean impacting the perf of non-debugger code for debugger functionality that is not normally used. Or, why can't we sort the Gc's list only when the debugger is enabled? Or, just iterate the whole list and hold off on fixing that pert bug until we had evidence that it's real. I think it would be bad if we had two lists for doing exactly the same thing.
(In reply to comment #5) > > Or we could sort the garbage collector’s list, but that would mean impacting the perf of non-debugger code for debugger functionality that is not normally used. > > Or, why can't we sort the Gc's list only when the debugger is enabled? The GC Executable list is a doubly linked list, not good for searching even if sorted. The GC CodeBlock list is a hash set, not good for sorting. So, we can’t just sort when we need it. > Or, just iterate the whole list and hold off on fixing that pert bug until we had evidence that it's real. When I first started work on the bytecode level breakpoints, I thought that a linear walk of all CodeBlocks may lead to a noticeable pause when first attaching the debugger with already previous defined breakpoints. But after working with the code thus far, I've gained new understanding of how the CodeBlocks get re-generated after the debugger attaches, and I think that the linear walk of all CodeBlocks might be tolerable after all. I will give it some more thought. > I think it would be bad if we had two lists for doing exactly the same thing. This sorted list is an AVLTree that is internal to the debugger only. I don’t think it’s such a bad thing, but I agree that it’d be good to avoid having a second data structure if the first will suffice.
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.