GNOME Bugzilla – Bug 619712
[patch] gjs debugger
Last modified: 2017-09-16 20:25:31 UTC
Attached is a patch which uses the SpiderMonkey debugger hooks to implement a gjs debugger. The debugger exposes a simple line protocol interface over a TCP socket (port 5580 by default). The network-facing parts run entirely in their own thread, with their own GMainContext and main loop. This means that interacting with the debugger is totally independent of the main thread executing JS and will never block. It doesn't require a glib main loop to be running in your gjs program. With the debugger, you can set and delete breakpoints based on line number, suspend and resume execution of the runtime, step through code line by line, view stack traces, examine local variables at any stack frame, and evaluate code at any stack frame. The debugger also supports the "debugger" JavaScript statement, and runtime execution will suspend just as if a breakpoint were hit when it comes across one of these. This is a really handy feature. Being a simple protocol on the socket, you can interact with it directly (as there are no clients yet). I typically do so with "rlwrap nc localhost 5580". Here are the commands: break <file> <line> - create a breakpoint delete <file> <line> - delete a breakpoint stop - suspends execution of the runtime ASAP continue - resume execution of the runtime immediately step - steps through a line of code stack - prints the current stack trace locals [frame] - prints local variables at the current or given stack frame eval [frame] <expr> - evaluates an expression at the current or given stack frame status - prints whether the runtime is running or suspended Output from these commands should be self-explanatory. Since the debugger is implemented using the SpiderMonkey hooks, it's a "soft" debugger that runs entirely in the context of the JS runtime. There is no ptrace support and no support for debugging native code. However, this debugger should work well in tandem with gdb if you need to. This also means that if you attempt to get a stack trace while in native code (like, the glib main loop) you will simply get an empty stack trace. You can still evaluate code, but it'll run in the global context. If you try to suspend execution (with the "stop" command) it can only do so the next time JS code is executed. In general, to use the debugger in gjs projects you can either pass --debugger to gjs-console, or call gjs_context_start_debugger() if you are running the gjs runtime yourself. I don't consider this a finished implementation. There are a bunch of caveats: (1) There are no clients for the protocol. You have to connect to the socket directly. There is some tedium in this, but it's not too bad. It is obvious that the protocol would need some tweaks to actually support a real client. It may also be a good idea to scrap the line protocol and just implement the V8 debugger protocol: http://code.google.com/p/v8/wiki/DebuggerProtocol which would theoretically give us existing V8 debugger front-ends for free. (2) There is only step-into functionality (equivalent to gdb's "step") but no step-over (gdb's "next") or step-out (gdb's "finish") yet. (3) The debugger_handle_message() function is embarrassingly ugly, and needs to be cleaned up. (4) Validation of inputs is not well handled; you can see some atoi() calls in there. If you pass in something bad you will likely crash your entire program. But I'm putting this out there because I've found it to be pretty useful in my own debugging, and to see whether others think it's the right approach to the problem.
Created attachment 162017 [details] [review] patch which implements a debugger Bah. bugzilla usability fail.
Review of attachment 162017 [details] [review]: ::: gjs/console.c @@ +39,3 @@ { "command", 'c', 0, G_OPTION_ARG_STRING, &command, "Program passed in as a string", "COMMAND" }, { "include-path", 'I', 0, G_OPTION_ARG_STRING_ARRAY, &include_path, "Add the directory DIR to the list of directories to search for js files.", "DIR" }, + { "debugger", 'd', 0, G_OPTION_ARG_NONE, &debugger, "Starts a debugger instance", NULL }, I'm not sure it's a great idea idea to add debugger related command line options to the gjs-console binary. However, I cannot find any good alternatives to it. That being said, it should be possible to reduce this to only one command, the port the debugger should be started on. If no port given; don't start the debugger. Instead of debugger-wait, require a 'debugger' keyword to be placed in the beginning of the execution. The port *value* could be made optional, to start on a default port. ::: gjs/context.c @@ +976,3 @@ +void +gjs_context_start_debugger(GjsContext *js_context, + int port, int -> uint16? @@ +977,3 @@ +gjs_context_start_debugger(GjsContext *js_context, + int port, + gboolean wait_for_connection) I think JSBool is more commonly used in gjs, at least when it comes to return values. @@ +980,3 @@ +{ + g_return_if_fail(GJS_IS_CONTEXT(js_context)); + g_return_if_fail(js_context->runtime); port should be validated here > 0, < 65535. ::: gjs/debugger.c @@ +48,3 @@ +typedef struct { + /* Network port debugger is listening on */ + int port; uint16 @@ +91,3 @@ + int start_line; + int end_line; +} GjsScript; GjsDebuggerScript perhaps? @@ +97,3 @@ + char *filename; + int line; +} GjsBreakpoint; and GjsDebuggerBreakpoint @@ +102,3 @@ + +static void +debugger_suspend_execution(GjsDebugger *debugger, const char *filename, int line) This is sometimes called with line=0 which is obviously not a valid source code line. That case should probably be handled, eg don't refer to filename.js:0 etc. @@ +157,3 @@ + debugger->current_context = ctx; + + gjs_script = g_new0(GjsScript, 1); g_slice_new @@ +164,3 @@ + gjs_script->end_line = line + JS_GetScriptLineExtent(ctx, script) - 1; + + /* Left over? @@ +200,3 @@ + } + + if (matching_script != NULL) { if (!matching_script) return; to reduce indentation @@ +245,3 @@ + gjs_debug(GJS_DEBUG_DEBUGGER, "New breakpoint created: %s %d\n", filename, line); + + breakpoint = g_new0(GjsBreakpoint, 1); g_slice_new @@ +265,3 @@ + + if (matching_script != NULL) { + /* Leftover? @@ +404,3 @@ + +static void +debugger_print_locals(GjsDebugger *debugger, int frame) Contain quite a bit of generic JS code that could be moved out of debugger.c @@ +449,3 @@ +static void +debugger_eval(GjsDebugger *debugger, int frame, const char *expression) +{ Ditto, generic code. @@ +494,3 @@ + +static void +debugger_send_message(GjsDebugger *debugger, const char *line) Could make this take a va_list, since most callers end up calling g_strdup_printf before. @@ +515,3 @@ + +static void +debugger_handle_message(GjsDebugger *debugger, const char *line) Please include the protocol spec in the source somewhere @@ +532,3 @@ + char *msg = g_strdup_printf("error invalid message \"%s\"\n", line); + debugger_send_message(debugger, msg); + g_free(msg); Missing return? @@ +540,3 @@ + char *msg = g_strdup_printf("error invalid message \"%s\"\n", line); + debugger_send_message(debugger, msg); + g_free(msg); Missing return as well. @@ +553,3 @@ + debugger_print_stacktrace(debugger); + } else if (strcmp(args[0], "locals") == 0) { + int frame = 0; Needs a bit better validation/error reporting here. @@ +560,3 @@ + debugger_print_locals(debugger, frame); + } else if (strcmp(args[0], "eval") == 0) { + int frame = 0; Ditto @@ +622,3 @@ +{ + GjsDebugger *debugger = user_data; + debugger->channel = NULL; Nothing to unref? @@ +632,3 @@ + /* Connection made, accept it */ + if (cond & G_IO_IN) { + int fd; separate function makes sense here. @@ +636,3 @@ + GSource *source; + + fd = accept(g_io_channel_unix_get_fd(listen_channel), Handle -1 here, I think @@ +643,3 @@ + /* Right now we only allow one client at a time */ + if (debugger->channel != NULL) { + const char *msg = "already-connected\n"; Why not use debugger_send_message() here? @@ +654,3 @@ + } + + source = g_io_create_watch(channel, Wait, we have two watches? One for accepting new clients and one for already connected ones? @@ +657,3 @@ + G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL); + g_source_set_callback(source, + (GSourceFunc) channel_watch_cb, gjs_debugger_client_channel_handler maybe? @@ +669,3 @@ + } + + if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) { Will G_IO_NVAL happen in practice? When will it happen? @@ +670,3 @@ + + if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) { + g_main_loop_quit(debugger->mainloop); Will everything be properly freed up here? @@ +686,3 @@ + GSource *source; + + fd = socket(AF_INET, SOCK_STREAM, 0); Set errno to -1 before just to be safe. @@ +688,3 @@ + fd = socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) { + printf("Error creating the socket! %s\n", strerror(errno)); Include 'debugger' somewhere in the error message, the same applies for the other errors. @@ +699,3 @@ + addr.sin_port = htons(debugger->port); + + if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { set errno here to as setsockopt can fail. @@ +721,3 @@ + G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL); + g_source_set_callback(source, + (GSourceFunc) listen_channel_watch_cb, Could use better naming, gjs_debugger_channel_source_func or so. @@ +745,3 @@ +} + +void This should probably wait/block until the thread has been successfully created and the socket communication channel has been established. Would be great to be able to set a GError as well. @@ +748,3 @@ +gjs_debugger_init(JSRuntime *runtime, int port, gboolean wait_for_connection) +{ + GjsDebugger *debugger = g_new0(GjsDebugger, 1); g_slice_new @@ +757,3 @@ + port = 5580; + + JS_SetNewScriptHook(runtime, debugger_new_script_handler, debugger); I'd prefer naming these 4 handlers consistently, for instance: gjs_debugger_new_script_hook gjs_debugger_destroy_script_hook gjs_debugger_debug_handler_hook gjs_debugger_interrupt_hook ::: gjs/jsapi-util-string.c @@ +585,3 @@ + * gjs_format_stack: + * @context: a JSContext + * @buf: a GString in which the results will be stored a #GString
(In reply to comment #2) > That being said, it should be possible to reduce this to only one > command, the port the debugger should be started on. > If no port given; don't start the debugger. I've tried this, but as far as I can tell there's no way with glib option parser to distinguish between a command-line flag being provided without a value and not being provided at all. There's no way in the GOptionEntry to set a default value. This seems like kind of a big oversight. Ideally: no --debugger provided - no debugger started --debugger - start debugger on default port --debugger=1234 - start debugger on default port
You should be able to do it with G_OPTION_ARG_CALLBACK: static gboolean debugger = FALSE; static int debugger_port = 5580; ... { "debugger", 'd', G_OPTION_FLAG_OPTIONAL_ARG, G_OPTION_ARG_CALLBACK, parse_debugger_option, "Starts a debugger instance, optionally on PORT", "PORT" }, ... static gboolean parse_debugger_option (const gchar *option_name, const gchar *value, gpointer data, GError **error) { debugger = FALSE; if (value) debugger_port = atoi (value); }
(In reply to comment #4) > { > debugger = FALSE; uh, TRUE, obviously
A few quick comments. I'll attach a WIP patch that addresses a lot -- but not all -- of your comments. (In reply to comment #2) > I'm not sure it's a great idea idea to add debugger related command line > options to the gjs-console binary. However, I cannot find any good alternatives > to it. That being said, it should be possible to reduce this to only one > command, the port the debugger should be started on. > If no port given; don't start the debugger. Instead of debugger-wait, require a > 'debugger' keyword to be placed in the beginning of the execution. The port > *value* could be made optional, to start on a default port. Ok, this is done based on Dan's code fragment. Thanks Dan! I've dropped the debugger-wait argument altogether. I have never used it in my use of the debugger. > ::: gjs/context.c > @@ +976,3 @@ > +void > +gjs_context_start_debugger(GjsContext *js_context, > + int port, > > int -> uint16? Done. > @@ +977,3 @@ > +gjs_context_start_debugger(GjsContext *js_context, > + int port, > + gboolean wait_for_connection) > > I think JSBool is more commonly used in gjs, at least when it comes to return > values. N/A, since wait_for_connection was dropped. > @@ +980,3 @@ > +{ > + g_return_if_fail(GJS_IS_CONTEXT(js_context)); > + g_return_if_fail(js_context->runtime); > > port should be validated here > 0, < 65535. done > ::: gjs/debugger.c > @@ +48,3 @@ > +typedef struct { > + /* Network port debugger is listening on */ > + int port; > > uint16 done > @@ +91,3 @@ > + int start_line; > + int end_line; > +} GjsScript; > > GjsDebuggerScript perhaps? done > @@ +97,3 @@ > + char *filename; > + int line; > +} GjsBreakpoint; > > and GjsDebuggerBreakpoint done > @@ +102,3 @@ > + > +static void > +debugger_suspend_execution(GjsDebugger *debugger, const char *filename, int > line) > > This is sometimes called with line=0 which is obviously not a valid source code > line. That case should probably be handled, > eg don't refer to filename.js:0 etc. The only time it would have been 0 was with wait_for_connection, and it was special cased. Since that was dropped, this isn't a concern anymore. > @@ +157,3 @@ > + debugger->current_context = ctx; > + > + gjs_script = g_new0(GjsScript, 1); > > g_slice_new done > @@ +164,3 @@ > + gjs_script->end_line = line + JS_GetScriptLineExtent(ctx, script) - 1; > + > + /* > > Left over? yeah, for debugging. removed. > @@ +200,3 @@ > + } > + > + if (matching_script != NULL) { > > if (!matching_script) return; to reduce indentation done > @@ +245,3 @@ > + gjs_debug(GJS_DEBUG_DEBUGGER, "New breakpoint created: %s %d\n", filename, > line); > + > + breakpoint = g_new0(GjsBreakpoint, 1); > > g_slice_new done > @@ +265,3 @@ > + > + if (matching_script != NULL) { > + /* > > Leftover? removed > @@ +404,3 @@ > + > +static void > +debugger_print_locals(GjsDebugger *debugger, int frame) > > Contain quite a bit of generic JS code that could be moved out of debugger.c Moved the stack walking and getting the stack's JSObject out into jsapi-utils.c > @@ +494,3 @@ > + > +static void > +debugger_send_message(GjsDebugger *debugger, const char *line) > > Could make this take a va_list, since most callers end up calling > g_strdup_printf before. done > @@ +515,3 @@ > + > +static void > +debugger_handle_message(GjsDebugger *debugger, const char *line) > > Please include the protocol spec in the source somewhere have not done yet, but wilco. > @@ +532,3 @@ > + char *msg = g_strdup_printf("error invalid message \"%s\"\n", > line); > + debugger_send_message(debugger, msg); > + g_free(msg); > > Missing return? fixed. (goto done) > @@ +540,3 @@ > + char *msg = g_strdup_printf("error invalid message \"%s\"\n", > line); > + debugger_send_message(debugger, msg); > + g_free(msg); > > Missing return as well. fixed. > @@ +553,3 @@ > + debugger_print_stacktrace(debugger); > + } else if (strcmp(args[0], "locals") == 0) { > + int frame = 0; > > Needs a bit better validation/error reporting here. not done yet, wilco. > @@ +622,3 @@ > +{ > + GjsDebugger *debugger = user_data; > + debugger->channel = NULL; > > Nothing to unref? No, because we unref immediately after creating the watch, and returning FALSE from the watch drops the last ref and destroys it. I'm not even sure debugger->channel still points at a valid object at this point. > @@ +632,3 @@ > + /* Connection made, accept it */ > + if (cond & G_IO_IN) { > + int fd; > > separate function makes sense here. done > @@ +636,3 @@ > + GSource *source; > + > + fd = accept(g_io_channel_unix_get_fd(listen_channel), > > Handle -1 here, I think done > @@ +643,3 @@ > + /* Right now we only allow one client at a time */ > + if (debugger->channel != NULL) { > + const char *msg = "already-connected\n"; > > Why not use debugger_send_message() here? It uses debugger->channel, and we are sending this message on the newly connected channel. debugger_send_message() is internal API, I didn't want to clutter it with having to take a channel object as an argument. > @@ +654,3 @@ > + } > + > + source = g_io_create_watch(channel, > > Wait, we have two watches? One for accepting new clients and one for already > connected ones? Correct. When you create a socket for listening and put it into non-blocking mode, poll() will give you a POLLIN event to indicate there are connections to accept(). Remember, there are two sockets: the one you listen() on, and the one you've accept()'d. (And there would be more, if we allowed more than one client at a time.) > @@ +657,3 @@ > + G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL); > + g_source_set_callback(source, > + (GSourceFunc) channel_watch_cb, > > gjs_debugger_client_channel_handler maybe? changed to debugger_client_channel_watch > @@ +669,3 @@ > + } > + > + if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) { > > Will G_IO_NVAL happen in practice? When will it happen? It can if a programmer closes the wrong socket and it happens to be ours. It seems to be general convention in glib programs to handle HUP | ERR | NVAL together, and better to handle it than not, IMO. (We'll spin at 100% CPU if it happens and we don't handle it, because the event will keep happening.) > @@ +670,3 @@ > + > + if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) { > + g_main_loop_quit(debugger->mainloop); > > Will everything be properly freed up here? No, but this is a catastrophic event that I can't imagine happening under normal circumstances. > @@ +686,3 @@ > + GSource *source; > + > + fd = socket(AF_INET, SOCK_STREAM, 0); > > Set errno to -1 before just to be safe. done > @@ +688,3 @@ > + fd = socket(AF_INET, SOCK_STREAM, 0); > + if (fd < 0) { > + printf("Error creating the socket! %s\n", strerror(errno)); > > Include 'debugger' somewhere in the error message, the same applies for the > other errors. haven't done this yet, but wilco. I'll probably end up changing all these printf()s to logs. > @@ +699,3 @@ > + addr.sin_port = htons(debugger->port); > + > + if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { > > set errno here to as setsockopt can fail. done > @@ +721,3 @@ > + G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL); > + g_source_set_callback(source, > + (GSourceFunc) listen_channel_watch_cb, > > Could use better naming, gjs_debugger_channel_source_func or so. Went with debugger_listen_channel_watch. > @@ +745,3 @@ > +} > + > +void > > This should probably wait/block until the thread has been successfully created > and the socket communication channel has been established. Would be great to be > able to set a GError as well. haven't looked at this yet, but I think it's doable. > @@ +748,3 @@ > +gjs_debugger_init(JSRuntime *runtime, int port, gboolean wait_for_connection) > +{ > + GjsDebugger *debugger = g_new0(GjsDebugger, 1); > > g_slice_new done > @@ +757,3 @@ > + port = 5580; > + > + JS_SetNewScriptHook(runtime, debugger_new_script_handler, debugger); > > I'd prefer naming these 4 handlers consistently, for instance: > gjs_debugger_new_script_hook > gjs_debugger_destroy_script_hook > gjs_debugger_debug_handler_hook > gjs_debugger_interrupt_hook I've been avoiding the gjs_ prefix because it's all internal to debugger.c. I went with: debugger_<foo>_handler, since they're not really "hooks" themselves, but they are all consistently named now. > ::: gjs/jsapi-util-string.c > @@ +585,3 @@ > + * gjs_format_stack: > + * @context: a JSContext > + * @buf: a GString in which the results will be stored > > a #GString done
Created attachment 164331 [details] [review] 2nd pass: includes many, but not all, of jdahlin's suggested fixes
BTW, if it's easier I can attach the interdiff between the first patch and this one, since they're separate git commits in my local tree.
(In reply to comment #4) > You should be able to do it with G_OPTION_ARG_CALLBACK: > > static gboolean debugger = FALSE; > static int debugger_port = 5580; > > ... > { "debugger", 'd', G_OPTION_FLAG_OPTIONAL_ARG, G_OPTION_ARG_CALLBACK, > parse_debugger_option, "Starts a debugger instance, optionally on PORT", "PORT" > }, > ... > > static gboolean > parse_debugger_option (const gchar *option_name, > const gchar *value, > gpointer data, > GError **error) > { > debugger = FALSE; > if (value) > debugger_port = atoi (value); > } Hmm, so this seems to fail in this case: gjs-console -d something.js because "something.js" is passed in as the value. AFAICT there's no way to tell the parser that no, this isn't an option and is really a part of the argv.
Created attachment 180507 [details] [review] 3rd revision of the debugger patch Ok, here's a new version of the patch which I believe takes into account all the existing comments. It improves the output from the "stack" command to be more machine readable. It adds "next" and "finish" commands which should make it much more useful.
Created attachment 181174 [details] [review] updated patch, against latest master This patch doesn't add any new functionality over the last version, but it is rebased against current master.
(In reply to comment #9) > Hmm, so this seems to fail in this case: > > gjs-console -d something.js > > because "something.js" is passed in as the value. AFAICT there's no way to > tell the parser that no, this isn't an option and is really a part of the argv. generally -- should do it, e.g. gjs-console -d -- something.js granted I didn't try it though...
The code in this patch depends on an API that isn't there anymore, but see bug 787573.