GNOME Bugzilla – Bug 721246
[RFC] Code Coverage and Debugger Support
Last modified: 2015-06-19 02:12:09 UTC
Gjs already includes an internal function profiler, but for testing purposes it would be incredibly useful to have support for generating coverage reports of code that was executed during tests. This patch series adds such support. Some general notes on the implementation: A large part of the work here was to add a suitable abstraction over SpiderMonkey's JS debugging hooks (e.g., JS_SetCallHook, JS_SetInterruptHook, JS_SetTrap/JS_ClearTrap, JS_SetNewScriptHook). We have multiple potential clients that might want to use these hooks and we need to have some sort of multiplexer to handle SpiderMonkey's state so that we don't have multiple potential clients fighting over it. This takes the form of GjsDebugInterruptRegister (exposed to clients through the GjsInterruptRegister interface. The way this works is that clients register a callback function to occur upon a specific SpiderMonkey debugging hook and they get a GjsDebugConnection object in return with unique and single ownership. Once they are done with a particular debugging hook, they just destroy the GjsDebugConnection object using gjs_debug_connection_unregister and SpiderMonkey's internal state will automatically revert back if it would be appropriate to do so. GjsProfiler and GjsDebugCoverage sit on top of this. The code coverage tool can be used by specifying -C on the console command line with a directory (including subdirectories) to be considered part of the coverage report. If a file is executed from within these directories, then coverage data will be generated for them. By default, we place all coverage data inside $(file).js.info, but it can be redirected to a single file using --coverage-output . The coverage data is in lcov format and can be turned into an HTML style coverage report with genhtml.
Created attachment 265052 [details] [review] Remove unused gjs_files_with_tests
Created attachment 265053 [details] [review] Fix context leak.
Created attachment 265054 [details] [review] Added GjsInterruptRegister interface and GjsDebugInterruptRegister object. This interface (and object) serve as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsInterruptRegister, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a GjsDebugConnection. This object is effectively just a handle for an active connection to the debugging interface. Once clients are done with a particular debugging hook they should unref their connection. This will automatically call back into the interrupt register's implementation and handle the proper tear down of that hook inside of SpiderMonkey if required.
Created attachment 265055 [details] [review] Integrate GjsProfiler with GjsInterruptRegister. GjsProfiler now takes a strong reference to GjsInterruptRegister and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Review of attachment 265052 [details] [review]: Yep.
Created attachment 265056 [details] [review] Added GjsCoverage object (and thus code coverage support). This object effectively just takes a connection to the single step handler and records how many times we hit each line. It then provides a function to write this data out to a GFile, or if the GFile provided is NULL, writes to $(file).js.info. To prevent clutter, paths considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a path to generate coverage reports for and using --coverage-output to write coverage data to a single file.
Review of attachment 265053 [details] [review]: I don't care too much about this, considering that the exit(); right there.
Review of attachment 265054 [details] [review]: Mostly a preliminary review. Will do a more detailed review after basic stuff is fixed. I notice a consistent style nit, though. You define functions like: void foo(int *foo, int bar); The standard GNOME style that gjs uses is to align like so: void foo(int *foo, int bar); ::: gjs/debug-connection.cpp @@ +24,3 @@ +{ + GjsDebugConnectionDisposeCallback callback; + gpointer user_data; This seems to be a heap-allocated GDestroyNotify. What's the motivation of this? ::: gjs/debug-interrupt-register.cpp @@ +59,3 @@ + GHashTable *new_script_connections; + + /* This is a hashtable of names to known scripts */ It's a hashtable of GjsDebugScriptLookupInfo to scripts @@ +63,3 @@ + + /* Observing Reference */ + GjsContext *context; What's an "observing reference"? @@ +91,3 @@ + +static void +gjs_debug_user_callback_asssign(GjsDebugUserCallback *user_callback, asssign => assign @@ +1108,3 @@ + +static void +search_for_script_with_closest_baseline_floor_callback(gpointer key, This doesn't seem to be doing anything? Leftover? ::: gjs/debug-interrupt-register.h @@ +63,3 @@ + + /*< private >*/ + GjsDebugInterruptRegisterPrivate *priv; Remove in favor of using get_instance_private everywhere. ::: gjs/executable-linesutil.cpp @@ +16,3 @@ + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + * + * Authored By: Sam Spilsbury <sam@endlessm.com> I'd prefer if this was written in pure JS and used Reflect.parse, or used the C++ API that Reflect.parse uses underneath. ::: gjs/interrupt-register.cpp @@ +88,3 @@ + +const gchar * +gjs_interrupt_info_get_filename(const GjsInterruptInfo *info) What's the point of these getters if the structure is public? ::: test/gjs-tests-add-funcs.h @@ +22,3 @@ + +void add_tests_for_debug_register (); +void add_tests_for_debug_connection (); I know this is a test binary, but I think these should be namespaced: void gjs_test_add_tests_for_debug_register (void); void gjs_test_add_tests_for_debug_connection (void);
Review of attachment 265055 [details] [review]: Looks good. ::: gjs/profiler.h @@ +31,2 @@ typedef struct _GjsProfiler GjsProfiler; +typedef struct _GjsInterruptRegister GjsInterruptRegister; You can't include interrupt-register.h or something? This opaque typedef has to be here?
(In reply to comment #7) > Review of attachment 265053 [details] [review]: > > I don't care too much about this, considering that the exit(); right there. It causestons of valgrind complaints. I know it doesn't make a runtime difference, but I'd prefer to keep the amount of leaked memory as low as possible at exit so that I can see at a glance if I'm doing something horrifically wrong and I don't think the fact that there is an exit() there is a good reason to reject this.
Review of attachment 265053 [details] [review]: Ah, yes, valgrind being silly is a thing. I think it will still leak if it takes the exit() case above, which is why I find it to be weird. We tend to have a "goto out;" strategy for this, or perhaps we should bite the bullet and start using the libgsystem cleanup macros? Anyway, this is fine for now.
Review of attachment 265056 [details] [review]: Oh, please don't use "gchar", "gint", etc. in new code. "char" and "int" are good enough. ::: gjs/context.cpp @@ +110,3 @@ PROP_GC_NOTIFICATIONS, + PROP_COVERAGE_PATHS, + PROP_COVERAGE_OUTPUT, I don't really like this API. I'd prefer to see something like: gjs_context_set_coverage_reporter (context, gjs_coverage_reporter_new (paths, output_path)); ::: gjs/coverage.cpp @@ +92,3 @@ + + while (str) + { gjs style uses brace-on-line: while (str) { ... } @@ +107,3 @@ + +static guint +count_lines_in_string(const gchar *data) Really? I mean, I know you compsci guys love your functional composition, but this is like five lines: static unsigned int count_lines_in_string(const char *data) { int lines = 0; for (; *data; data++) if (*data == '\n') lines++; return lines; } @@ +125,3 @@ + &lines, + &length, + NULL)) Keep this all on one line. @@ +127,3 @@ + NULL)) + { + g_critical("Filename %s does not have any contents!", filename); Not sure why this is a critical. @@ +143,3 @@ + +static void +mark_executable_lines(GArray *statistics, What does it mean to "mark executable lines"? @@ +161,3 @@ + GHashTable *file_statistics = (GHashTable *) user_data; + if (g_hash_table_contains(file_statistics, + filename)) Keep all this on one line. @@ +208,3 @@ + const gchar *no_function_coverage = + "FNF:0\n"\ + "FNH:0\n"; I assume this and no_branch_coverage below are TODOs? @@ +259,3 @@ + g_data_output_stream_put_string(data_stream, "DA:", NULL, NULL); + print_uint32_into_stream(data_stream, i); + g_data_output_stream_put_string(data_stream, ",", NULL, NULL); I'd prefer if you used g_strdup_printf here. @@ +305,3 @@ +{ + gsize tracefile_name_buffer_size = strlen(script_name) + 8; + gchar tracefile_name_buffer[tracefile_name_buffer_size]; This is a gcc extension that we don't use. Either use alloca(); or just use g_strdup_printf(); because who really cares. @@ +321,3 @@ + g_return_val_if_fail(g_seekable_can_seek(seekable), ostream); + + if (!g_seekable_seek(seekable, 0, (GSeekType) SEEK_END, NULL, &error)) why not use G_SEEK_END? @@ +361,3 @@ + /* Unreferencing the tracefile is safe here as the + * underlying GFileOutputStream does not have + * a weak reference to it */ "Weak reference" tends to be a very specific thing in GObject terms. Just remove the comment. It's expected that if the OutputStream wants the file to exist beyond the creator, it will hold onto it. @@ +433,3 @@ + /* get_appropriate_tracefile_ref will automatically set the write + * pointer to the correct place in the file */ + GFileOutputStream *ostream = get_appropriate_tracefile_ref(statistics_print_data->specified_ostream, ANSI C. Declarations go at the top of blocks. @@ +441,3 @@ + return; + + GDataOutputStream *data_stream = data_output_stream_new_no_close_base(G_OUTPUT_STREAM(ostream)); Why do you want to not close on unref? @@ +552,3 @@ + GFileInfo *current_file = g_file_enumerator_next_file(enumerator, NULL, NULL); + + while (current_file) while ((current_file = g_file_enumerator_next_file (enumerator, NULL, NULL)) { } Colin also wrote gs_file_enumerator_iterate, which helps in the error case. libgsystem is really awesome, we should start using it. https://git.gnome.org/browse/libgsystem/tree/gsystem-file-utils.c#n864
> Review of attachment 265054 [details] [review]: > > Mostly a preliminary review. Will do a more detailed review after basic stuff > is fixed. > > I notice a consistent style nit, though. You define functions like: > > void foo(int *foo, > int bar); > > The standard GNOME style that gjs uses is to align like so: > > void foo(int *foo, > int bar); Ah, didn't pick up on that. Thanks. > > ::: gjs/debug-connection.cpp > @@ +24,3 @@ > +{ > + GjsDebugConnectionDisposeCallback callback; > + gpointer user_data; > > This seems to be a heap-allocated GDestroyNotify. What's the motivation of > this? It basically is that. The GDestroyNotify needs to have the user data provided by the GjsDebugInterruptRegister in order to actually perform its work, so its probably better to give clients one pointer rather than two ;-) Also, it makes sense for clients to "unregister" a connection as opposed to calling some arbitrary function. > > ::: gjs/debug-interrupt-register.cpp > @@ +59,3 @@ > + GHashTable *new_script_connections; > + > + /* This is a hashtable of names to known scripts */ > > It's a hashtable of GjsDebugScriptLookupInfo to scripts Yep. > > @@ +63,3 @@ > + > + /* Observing Reference */ > + GjsContext *context; > > What's an "observing reference"? Its probably a C++ism ;-) Observing is just some jargon for "a reference that does not take any ownership". I can remove this comment if it is confusing. > > @@ +91,3 @@ > + > +static void > +gjs_debug_user_callback_asssign(GjsDebugUserCallback *user_callback, > > asssign => assign Oh dear. > > @@ +1108,3 @@ > + > +static void > +search_for_script_with_closest_baseline_floor_callback(gpointer key, > > This doesn't seem to be doing anything? Leftover? Oh dear. I think that may have been lost in a rebase :( I'm surprised that all the tests are still passing. > > ::: gjs/debug-interrupt-register.h > @@ +63,3 @@ > + > + /*< private >*/ > + GjsDebugInterruptRegisterPrivate *priv; > > Remove in favor of using get_instance_private everywhere. Sure. (The preferred style here keeps changing every year :P) > > ::: gjs/executable-linesutil.cpp > @@ +16,3 @@ > + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. > + * > + * Authored By: Sam Spilsbury <sam@endlessm.com> > > I'd prefer if this was written in pure JS and used Reflect.parse, or used the > C++ API that Reflect.parse uses underneath. Hmm, that's interesting. Do you know where I can find that API? > > ::: gjs/interrupt-register.cpp > @@ +88,3 @@ > + > +const gchar * > +gjs_interrupt_info_get_filename(const GjsInterruptInfo *info) > > What's the point of these getters if the structure is public? The struct definitely should not be public. If it is, I'll make it private. Possibly lost in a recent rebase as well. > > ::: test/gjs-tests-add-funcs.h > @@ +22,3 @@ > + > +void add_tests_for_debug_register (); > +void add_tests_for_debug_connection (); > > I know this is a test binary, but I think these should be namespaced: > > void gjs_test_add_tests_for_debug_register (void); > void gjs_test_add_tests_for_debug_connection (void); Ack.
(In reply to comment #13) > Hmm, that's interesting. Do you know where I can find that API? https://developer.mozilla.org/en-US/docs/SpiderMonkey/Parser_API
(In reply to comment #12) > Review of attachment 265056 [details] [review]: > > Oh, please don't use "gchar", "gint", etc. in new code. "char" and "int" are > good enough. Ah. The preferred style here seems to have changed again ;-) > > ::: gjs/context.cpp > @@ +110,3 @@ > PROP_GC_NOTIFICATIONS, > + PROP_COVERAGE_PATHS, > + PROP_COVERAGE_OUTPUT, > > I don't really like this API. I'd prefer to see something like: > > gjs_context_set_coverage_reporter (context, gjs_coverage_reporter_new (paths, > output_path)); In fact, I didn't either. I only had it this way to make it consistent with how GjsProfiler works (e.g., the profiler and other objects are owned by the context. But in fact it makes much more sense to have the coverage object itself (and perhaps the profiler?) injected into GjsContext. If you think it would be better this way, I'd be glad to change it. One question: do you think it would be better to inject GjsDebugCoverage as a > > ::: gjs/coverage.cpp > @@ +92,3 @@ > + > + while (str) > + { > > gjs style uses brace-on-line: > > while (str) { > ... > } Hmm, the surrounding code seemed to disagree with that. However, maybe I was only looking at parts that were of bad style, so I can rewrite this if need be. > > @@ +107,3 @@ > + > +static guint > +count_lines_in_string(const gchar *data) > > Really? I mean, I know you compsci guys love your functional composition, but > this is like five lines: > > static unsigned int > count_lines_in_string(const char *data) > { > int lines = 0; > for (; *data; data++) > if (*data == '\n') > lines++; > return lines; > } I've read multiple books about crafting maintainable code and every single one of them has stated that a preference should be made for small functions as opposed to leaking implementation details into functions that work on a different level of abstraction. Perhaps we disagree on the preferred style here and I'd be happy to change it if need be. > > @@ +125,3 @@ > + &lines, > + &length, > + NULL)) > > Keep this all on one line. OK. > > @@ +127,3 @@ > + NULL)) > + { > + g_critical("Filename %s does not have any contents!", filename); > > Not sure why this is a critical. Hmm. I think when I wrote this I considered it to be an error condition (No Contents? read() probably failed). However, now that you mention it, it could also be a legitimate case. So I'll remove it. > > @@ +143,3 @@ > + > +static void > +mark_executable_lines(GArray *statistics, > > What does it mean to "mark executable lines"? Yep the naming of this function needs to improve. Basically, it finds all executable lines in a script and sets the counter for them to 0 instead of -1. > > @@ +161,3 @@ > + GHashTable *file_statistics = (GHashTable *) user_data; > + if (g_hash_table_contains(file_statistics, > + filename)) > > Keep all this on one line. > > @@ +208,3 @@ > + const gchar *no_function_coverage = > + "FNF:0\n"\ > + "FNH:0\n"; > > I assume this and no_branch_coverage below are TODOs? Yes. Detecting individual branches and functions is not a super-trivial task and line coverage was the most important thing for us at the moment. > > @@ +259,3 @@ > + g_data_output_stream_put_string(data_stream, "DA:", NULL, NULL); > + print_uint32_into_stream(data_stream, i); > + g_data_output_stream_put_string(data_stream, ",", NULL, NULL); > > I'd prefer if you used g_strdup_printf here. Interesting. I had it like that before but Cosimo didn't like it :P I can change it back to g_strdup_printf if need be. Removes the need to have a GDataOutputStream around too. > > @@ +305,3 @@ > +{ > + gsize tracefile_name_buffer_size = strlen(script_name) + 8; > + gchar tracefile_name_buffer[tracefile_name_buffer_size]; > > This is a gcc extension that we don't use. Either use alloca(); or just use > g_strdup_printf(); because who really cares. Ah, was not aware that it was a gcc extension. I can use g_strdup_printf. > > @@ +321,3 @@ > + g_return_val_if_fail(g_seekable_can_seek(seekable), ostream); > + > + if (!g_seekable_seek(seekable, 0, (GSeekType) SEEK_END, NULL, &error)) > > why not use G_SEEK_END? Typo it seems :) > > @@ +361,3 @@ > + /* Unreferencing the tracefile is safe here as the > + * underlying GFileOutputStream does not have > + * a weak reference to it */ > > "Weak reference" tends to be a very specific thing in GObject terms. Just > remove the comment. It's expected that if the OutputStream wants the file to > exist beyond the creator, it will hold onto it. OK. (I'm from C++ land where it means "observer"). > > @@ +433,3 @@ > + /* get_appropriate_tracefile_ref will automatically set the write > + * pointer to the correct place in the file */ > + GFileOutputStream *ostream = > get_appropriate_tracefile_ref(statistics_print_data->specified_ostream, > > ANSI C. Declarations go at the top of blocks. OK (although Gjs is now written in C++ ;-)) > > @@ +441,3 @@ > + return; > + > + GDataOutputStream *data_stream = > data_output_stream_new_no_close_base(G_OUTPUT_STREAM(ostream)); > > Why do you want to not close on undef? You've picked up on the need to comment this, but I'll explain here, since it might actually be a bug in GOutputStream. Basically, if one type of GOutputStream operates on top of a different type of GOutputStream (for instance, GDataOutputStream on top of GFileOutputStream) then closing the first one will implicitly close the second. We don't want that because we need to keep the GFileOutputStream open until its last reference is dropped, hence the reason why we don't want to auto-close the GDataOutputStream. > > @@ +552,3 @@ > + GFileInfo *current_file = g_file_enumerator_next_file(enumerator, NULL, > NULL); > + > + while (current_file) > > while ((current_file = g_file_enumerator_next_file (enumerator, NULL, NULL)) > { > } > > Colin also wrote gs_file_enumerator_iterate, which helps in the error case. > libgsystem is really awesome, we should start using it. > > https://git.gnome.org/browse/libgsystem/tree/gsystem-file-utils.c#n864 Cool, didn't know about that (G* is massive). I'll use that instead.
> One question: do you think it would be better to inject GjsDebugCoverage as a Whoops, didn't finish that sentence. Do you think it would be better to inject GjsDebugCoverage as an object construction property? It really only needs to be set once and I tend to avoid having _set_foo () if I can.
(In reply to comment #16) > Do you think it would be better to inject GjsDebugCoverage as an object > construction property? It really only needs to be set once and I tend to avoid > having _set_foo () if I can. Personally, I don't think of coverage as a toggle on a GjsContext. The GjsContext is where your program "lives", so it feels conceptually wrong to have a toggle button on the GjsContext. It feels more like an addon to the context, which adds reporting capabilities. I also think there might be potential for testing a context somebody else created and adding coverage functionality to it. Though I'm conflicted about an API for this. GjsContext, essentially, is a wrapper for JSContext. Mozilla has changed their opinions about what a JSContext is and what a JSRuntime is over time, after compartments came into play. So I'm not sure if it makes sense for a tool to create a GJS Context and start your running stuff in it with reporting turned on, or if an app is supposed to create a GJS Context, and then you pass it to the coverage reporting tool which adds its own stuff. It gets a bit more confusing with gnome-shell since it's so different from most other gjs-using apps, where it has a lot more C code and the initialization and main cycle is more complex.
(In reply to comment #15) > Yep the naming of this function needs to improve. > > Basically, it finds all executable lines in a script and sets the counter for > them to 0 instead of -1. What does 0 and -1 mean? How is this structured? A comment above the appropriate data structures and methods will make it more clear.
(In reply to comment #18) > (In reply to comment #15) > > Yep the naming of this function needs to improve. > > > > Basically, it finds all executable lines in a script and sets the counter for > > them to 0 instead of -1. > > What does 0 and -1 mean? How is this structured? A comment above the > appropriate data structures and methods will make it more clear. OK.
I'm part-way done with cleaning this up, although there are some larger-ticket items left to do. You can track the progress here: https://github.com/smspillaz/gjs/pull/1 Basically: 1. Use SpiderMonkey's reflection API to hunt out non-executable lines 2. Style nitpicks (braces fall on the same line as flow-control conditions), alignment 3. Fixing up the ownership fiasco, which I think we resolved to: GjsContext owns (and composes) GjsInterruptRegister (to be renamed to GjsDebugHooks) The console/shell/libpeas/whatever owns the "tools" which in turn have an observing reference over the context and context's interrupt register. So GjsContext will no longer own GjsProfiler (the shell will own that, and perhaps the console) and the console will own the coverage tool.
Created attachment 265496 [details] [review] Added GjsReflectedScript and GjsReflectedExecutableScript. GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines. GjsReflectedExecutableScript is an implementation of the former which can be updated as-needs-be at runtime (while new executable lines are added).
Created attachment 265497 [details] [review] Added GjsDebugHooks interface and GjsMultiplexedDebugHooks object. This interface (and object) serve as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsDebugHooks, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a GjsDebugConnection. This object is effectively just a handle for an active connection to the debugging interface. Once clients are done with a particular debugging hook they should unref their connection. This will automatically call back into the interrupt register's implementation and handle the proper tear down of that hook inside of SpiderMonkey if required.
Created attachment 265498 [details] [review] Integrate GjsProfiler with GjsInterruptRegister. GjsProfiler now takes a strong reference to GjsInterruptRegister and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Created attachment 265499 [details] [review] Added GjsCoverage object (and thus code coverage support). This object effectively just takes a connection to the single step handler and records how many times we hit each line. It then provides a function to write this data out to a GFile, or if the GFile provided is NULL, writes to $(file).js.info. To prevent clutter, paths considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a path to generate coverage reports for and using --coverage-output to write coverage data to a single file. Pass --accumulate-coverage if you want coverage data to be appended to the same file. This is useful for some test runners.
I've attached a newer rebased patch set. Main highlights are: 1. Relevant fixups and style nits (hopefully all of them) 2. Dropped the JS hand-parser and usage of JS_GetLinePCs entirely. JS_GetLinePCs doesn't return entirely useful information for us and correcting it was error prone at best. We're now using a wrapper around Reflect.parse. Incidentally, this made doing branch and function coverage possible so we're now doing that too. 3. Coverage tool is now owned by the console. Something of note: I'm not sure if my patch earlier to unref the context should be applied. I've noticed that at least when testing this tool on a few applications that there are some situations where incorrect GObject usage can lead to invalid state ending up in the garbage collector which leads to mysterious-looking crashes there. Gjs doesn't warn about this (though I am filing bug reports for those). Obviously, we have to do a garbage-collection on context destruction. Having the test driver report that all your tests failed in coverage mode isn't exactly ideal, so maybe we should just let it leak for now (or enable a "clean-up-memory-on-exit" environment variable, as absolutely horrible as that sounds?)
Review of attachment 265498 [details] [review]: I assume this commit message needs updating?
Review of attachment 265496 [details] [review]: ::: Makefile-modules.am @@ +17,3 @@ modules/promise.js \ + modules/format.js \ + modules/info_reflect.js This is going to need updating for the new gresource modules. ::: gjs/reflected-executable-script.cpp @@ +1,2 @@ +/* + * Copyright © 2013 Endless Mobile, Inc. Can you update the headers to say 2014? @@ +60,3 @@ + G_ADD_PRIVATE(GjsReflectedExecutableScript) + G_IMPLEMENT_INTERFACE(GJS_TYPE_REFLECTED_SCRIPT_INTERFACE, + gjs_reflected_executable_script_interface_init)) Any motivation for wanting to split things out into an interface? @@ +70,3 @@ +static GParamSpec *properties[PROP_N]; + +static void perform_reflection_if_necessary(GjsReflectedExecutableScript *script); Hm, I'm not a giant fan of the name here. We tend to call these sorts of "if_necessary" functions "ensure_blah", so "ensure_reflection_info", but the "ensure" name tends to signal to me something minor (allocating memory, not running a full parse and JS run). So, I'm not going to push it. What you have now is fine. @@ +76,3 @@ +{ + GjsReflectedExecutableScript *executable_script = GJS_REFLECTED_EXECUTABLE_SCRIPT(script); + GjsReflectedExecutableScriptPrivate *priv = (GjsReflectedExecutableScriptPrivate *) gjs_reflected_executable_script_get_instance_private(executable_script); grr, I hate the lack of implicit void* casts in C++ @@ +102,3 @@ +static const unsigned int * +gjs_reflected_executable_script_executable_lines(GjsReflectedScript *script, + unsigned int *n) "executable_lines" is a weird method name. Perhaps "get_executable_lines"? Even though it's static, documentation about what "n" and the return value are would be nice. Maybe make them both out variables so you can name both of them? @@ +125,3 @@ + const char *name) +{ + JSObject *propertyObject = NULL; We tend to use property_object, etc. in C code. @@ +140,3 @@ + +static GArray * +get_array_from_js_value(JSContext *context, Hm, I thought we had a helper function like this already, but I guess not. I like it, though. To match the style of other helper functions, though, it should return a boolean (either JSBool or gboolean), and it should give the array as an output member. When it returns FALSE, it should have called gjs_throw. That makes it super easy to compose: if (!get_array_from_js_value(context, ...)) return FALSE; @@ +142,3 @@ +get_array_from_js_value(JSContext *context, + jsval *value, + size_t array_element_size, We line up the names, not the declarations. foo (JSContext *context, int bar); @@ +149,3 @@ + + if (!JS_IsArrayObject(context, js_array)) { + g_critical("Returned object from is not an array"); Use gjs_throw, not g_critical here. @@ +153,3 @@ + } + + GArray *return_array = g_array_sized_new(TRUE, TRUE, array_element_size, 10); /* Chosen by fair dice roll. */ @@ +168,3 @@ + } + + if (!((*inserter)(return_array, context, &element))) { We tend to do foo() rather than (*foo)(); with function pointer invocations. @@ +179,3 @@ + +static GArray * +call_js_function_for_array_return(JSContext *context, This function would be unnecessary with the helper function redone, since it's super simple to compose: jsval rval; if (!JS_CallFunctionName(context, obj, function_name, 1, ast, &rval)) return FALSE; if (!get_array_from_js_value(context, &rval, sizeof(char *), clear_string, &array)) return FALSE; @@ +190,3 @@ + if (!JS_CallFunctionName(context, object, function_name, 1, ast, &rval)) { + gjs_log_exception(context); + g_critical("Failed to call %s", function_name); We already logged an exception, no need to do anything else. @@ +227,3 @@ + g_array_append_val(array, utf8_string_copy); + + JS_free(context, utf8_string); This is really interesting. It seems that in https://git.gnome.org/browse/gjs/commit/gjs/jsapi-util-string.cpp?id=5b41c10f64e32fe461d7b797c00acda4f9301514 we changed to using JS_EncodeStringToUTF8, which returns something allocated by jsapi. However, all existing callers assume it's created with g_malloc, and free it with g_free. I'm not sure what happens if mozjs is built with another malloc like jemalloc; probably a crash. However, it seems "safe" enough, and existing callers seem to rely on the behavior. In the mean time, just return utf8_string without taking a copy -- if it turns out to be a problem, we'll have to fix gjs_string_to_utf8. I've filed https://bugzilla.gnome.org/show_bug.cgi?id=721678 about this. @@ +453,3 @@ + GjsReflectedExecutableScriptPrivate *priv = (GjsReflectedExecutableScriptPrivate *) gjs_reflected_executable_script_get_instance_private(script); + + if (G_LIKELY(priv->reflection_performed)) Hm. G_LIKELY implies around 90/10% split. Given that this is really only called three times, with one of those times this being TRUE, that's a 33%/70% split, so I'd imagine the branch predictor being able to do a better job about this. @@ +458,3 @@ + /* It appears that we only want one current context at a time, so we need + * to explicitly make any current context non-current so that we can briefly + * use ours. Back up the current context and restore it later */ Why do we want a brand new context here? So we don't pollute globals? I'd rather do this without context shenanigans. @@ +465,3 @@ + "const InfoReflect = imports.info_reflect;\n" + "const ReflectOptions = {\n" + " loc: true\n" Seems unnecessary to do this in the bootstrap script to me. If we're not really going to support anything generic here, might as well just hardcode it in infoReflect.js. @@ +468,3 @@ + "};\n"; + + if (!gjs_context_eval(internal_context, This is probably something we would want gjs_eval_with_scope (see the commit in wip/reflect I linked above) for. JSObject *scope = JS_NewObject(context, NULL, NULL, NULL); JS_SetProperty(context, scope, "script", &script_val); if (!gjs_eval_in_scope(context, scope, "Reflect.parse(this.script, { loc: true })", -1, "<reflection>", &ast_value)) goto out; ... ::: gjs/reflected-script.cpp @@ +16,3 @@ + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + * + * Authored By: Sam Spilsbury <sam.spilsbury@canonical.com> This will need updating, I assume. ::: modules/info_reflect.js @@ +1,1 @@ +/* This should be infoReflect.js, given our typical naming patterns (e.g. byteArray) @@ +18,3 @@ + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. Missing "Authored by:" @@ +21,3 @@ + */ + +function removeShebangsAndParse(script, options) { Hm. https://git.gnome.org/browse/gjs/commit/?h=wip/require&id=b088f1ad235f03d40e90a2642cdf31b26b1e0511 This is in a branch that hasn't landed called wip/require, where I wrote strip_unix_shebangs. Maybe we can rewrite some of this commit to put strip_unix_shebangs in jsapi-util.cpp or some other place like that, and then just call Reflect.parse from C? @@ +31,3 @@ + +function getSubNodesForNode(node) { + let subNodes = []; Hm. It seems that sometimes we do: case 'A': return [node.foo]; case 'B': subNodes = [node.bar]; return subNodes; case 'C': subNodes.push(node.baz); return subNodes; I'd prefer it if we picked a style and stuck to it. I don't really care which. Review comments below assume we're picking C, but anything is fine. @@ +69,3 @@ + return [node.left, node.right]; + case 'ObjectExpression': + node.properties.forEach(function(prop) { subNodes.push.apply(node.properties.mapfunction(prop) { return prop.value; })); @@ +78,3 @@ + case 'NewExpression': + case 'CallExpression': + subNodes = subNodes.concat(node.arguments); subNodes.push.apply(node.arguments); is more efficient. @@ +80,3 @@ + subNodes = subNodes.concat(node.arguments); + subNodes.push(node.callee); + Remove whitespace. @@ +97,3 @@ + return subNodes; + case 'SwitchStatement': + subNodes = []; When could subNodes not already be empty? @@ +98,3 @@ + case 'SwitchStatement': + subNodes = []; + for (let i = 0; i < node.cases.length; i++) { I tend to use: for (let case of node.cases) { } now. for...of is new in ES6, and fully supported by mozjs. @@ +113,3 @@ + node.declarations.forEach(function (declarator) { + if (declarator.init != null) { + subNodes.push(declarator.init); No braces. @@ +123,3 @@ +} + +function collectForSubNodes(subNodes, collector) { I'd prefer if this was integrated with subNodes above, instead of being separate. So you'd do: let functionNames = collectFromAST(toplevelNode, collectFunctionNames); and this function would do the rest and aggregate the results. @@ +124,3 @@ + +function collectForSubNodes(subNodes, collector) { + let collection = []; "collection" has a standard meaning. Maybe "results"? @@ +129,3 @@ + subNodes.forEach(function(expression) { + let subCollection = collector(expression); + if (subCollection !== undefined) { I'd prefer it if we were consistent between != and !== in this file. I prefer !== in general, even though we tend not to use it in gjs/gnome-shell, so just use !==... (also, no braces) @@ +146,3 @@ + functionNames.push(node.id.name); + } + /* If the function wasn't found, we just push a name grr... the JS engine has ways of figuring out a function's name based on anonymous expressions like this: let x = function() { }; but it seems that's part of the compiler, not the parser. Oh well, this should still be fine. @@ +181,3 @@ + * a block statement then handle the case and go + * to the first line where execution starts */ +function getBranchExecutionStartLine(executionNode) { Is it "executable" or "execution"? And shouldn't this take a branchNode, not an executionNode? @@ +182,3 @@ + * to the first line where execution starts */ +function getBranchExecutionStartLine(executionNode) { + whitespace @@ +194,3 @@ + * that never actually get to executable code by handling + * all statements within a block */ + for (let statement of executionNode.body) { Oh hey, you know about for...of! @@ +224,3 @@ + case 'EmptyStatement': + case 'LabelledStatement': + return -1; Hm, seems unrelated to branches. Maybe this should be split out into isNodeExecutable which would do this test. @@ +241,3 @@ + let branches = []; + + let alternatesForThisBranch = []; The typical parser vocabulary for this when building a flow graph is a block's "exits". So, let branchExits = [];? @@ +246,3 @@ + insertExecutionStartLineIfExecutable(alternatesForThisBranch, node.consequent) + if (node.alternate !== null) + insertExecutionStartLineIfExecutable(alternatesForThisBranch, node.alternate); This function name is a bit hard to parse. Maybe do it like this: branchExits.push(node.consequent); if (node.alternate !== null) branchExits.push(node.alternate); ... let branchExitStartLines = branchExits.map(function(exitNode) { if (isNodeExecutable(exitNode)) return getBranchExitStartLine(exitNode); else return -1; })); @@ +264,3 @@ + if (alternatesForThisBranch.length) { + branches.push(createBranchInfo(node.loc.start.line, + alternatesForThisBranch)) Simply including this inline might be clearer. With the other AST walking cleanups, this would simply look like: return { line: node.loc.start.line, exits: branchExitStartLines }; @@ +279,3 @@ +} + +function executableExpressionLinesForNode(statement) { Would be great to have a comment describing *what* an "executable expression line" is. Also, shouldn't this take "node", not "statement"? @@ +282,3 @@ + let allExecutableExpressionLineNos = []; + + /* Only got an undefined statement */ Is there a valid place this could happen? @@ +291,3 @@ + statement.type.indexOf('Clause') !== -1 || + statement.type.indexOf('Literal') !== -1 || + statement.type.indexOf('Identifier') !== -1) { ugh @@ +314,3 @@ +function executableExpressionLinesForAST(ast) { + let allExpressions = collectForSubNodes(ast.body, executableExpressionLinesForNode); + /* Deduplicate the list */ I know I complain about too many functions, but I think this would make sense as a separate utility: function deduplicate(arr);
Review of attachment 265496 [details] [review]: Oh, and as a small nit, we tend not to put "." at the end of commit messages, and instead try to make them action phrase. This isn't a great commit message, but: Add some infrastructure reflection for code coverage purposes Code coverage tooling needs to associate high-level code concepts (branches, functions, executable lines) with low-level details like line numbers. Thankfully, Mozilla gives us the Reflect.parse API, which can parse JavaScript and provide us with detailed location information for our tooling. Add a new module, infoReflect, which contains some helpers for walking the JavaScript AST and extracting branches and function names. Also, add new C modules to invoke Reflect.parse and our helpers in the infoReflect module, for convenient use from our coverage tooling.
Review of attachment 265499 [details] [review]: ::: gjs/coverage.cpp @@ +272,3 @@ + g_hash_table_replace(file_statistics->functions, + g_strdup(function_name), + GINT_TO_POINTER(hit_count)); Yikes. There's no guarantee that two functions won't have the same name: function foo() { return (function test() { return "foo"; }); } function bar() { return (function test() { return "bar"; }); } foo(); bar(); As I understand it, this will emit FN:test:2, even though each test(); got called once. It seems the format we emit coverage reports in doesn't really have anything better, so I'm wondering if it makes sense to have function name reporting at all. Maybe this is such an edge case that it doesn't really matter. Maybe it makes sense to emit a warning in the results if we have the same function name recorded for two different line numbers. Hm... what's the lesser of the two evils: potentially inaccurate data, or no data at all? There should probably be a test case for this behavior. @@ +283,3 @@ + * A value of -1 for an element means that the line is non-executable and never actually + * reached. A value of 0 means that it was executable but never reached. A positive value + * indicates the hit count. Should probably mention the (obvious) reason for actually caring about non-executable vs. executable lines: we only care about coverage for executable lines, and so we don't want to report coverage misses for non-executable lines. @@ +291,3 @@ + * case we can neatly handle the error by marking that line executable. A hit on a line + * we thought was non-executable is not as much of a problem as noise generated by + * ostensible "misses" which could in fact never be executed. Just curious: are there any known cases where we'll receive data for a supposed non-executable line? Could we have a test case? @@ +301,3 @@ + + /* We are ignoring the zeroth line, so we want line_count + 1 */ + g_array_set_size(line_statistics, line_count + 1); I think you can use g_array_new_sized as a shortcut here. @@ +311,3 @@ + &n_executable_lines); + + /* In order to determine which lines are executable to start off with, we should take we should take => we take @@ +324,3 @@ +} + +/* Again we are creating a 1-1 representation of script lines to potential branches I find that wording like "Again" tends to read awkwardly in a doc comment, since I rarely read code top to bottom. You might want to say "Just like with create_line_coverage_statistics_from_reflection above, ..." @@ +409,3 @@ + filename); + + /* No current value exists, open the file and create statistics for If I have a comment explaining the condition/"innards" of an if statement, I tend to put the comment on the inside as well. @@ +491,3 @@ + unsigned int *n_functions_hit) +{ + FunctionHitCountData data = { Again, I tend to prefer GHashTableIter over building closures by hand. @@ +690,3 @@ +delete_file_and_open_anew(GFile *file, GError **error) +{ + return g_file_replace(file, Seems like sort of a weird wrapper for me. Just move it inline with the function. @@ +713,3 @@ + if ((*error)->code == G_IO_ERROR_EXISTS) { + g_clear_error(error); + ostream = g_file_append_to(file, What's wrong with just using this up-front? g_file_append_to documentation says it will create the file if it doesn't exist. Again, just move inline. @@ +746,3 @@ + + if (!g_seekable_seek(seekable, 0, (GSeekType) G_SEEK_END, NULL, &error)) + g_print("Failed to seek %s to end position : %s\n", Use g_warning here. @@ +747,3 @@ + if (!g_seekable_seek(seekable, 0, (GSeekType) G_SEEK_END, NULL, &error)) + g_print("Failed to seek %s to end position : %s\n", + "unknown", ugh. Just remove this slot, I guess. If you want to be fancy, you can use g_file_output_stream_query_info, but I doubt you will ever hit this case. @@ +752,3 @@ + +static GOutputStream * +get_appropriate_tracefile_ref(GFileOutputStream *specified_tracefile, Remove the _ref at the end here, since that usually means "add a refcount to this object". @@ +758,3 @@ + /* If we provided a tracefile, then just increase the refcount. It will + * be unreferenced later. We want to make sure we're at the end + * of the stream too */ No need to comment on refcounting. Is there ever a reason where you *wouldn't* be at the end of the stream if it's one we've kept? Did I miss a seek somewhere, or is it a sort of "just in case" thing? @@ +791,3 @@ + /* Unreferencing the tracefile is safe here as the + * underlying GFileOutputStream does not have + * an observing reference to it */ The comment here is only confusing. This is fairly standard GObject: this function "owns" the object GFile it created it, we pass it to ostream which takes a reference (since it needs it for itself), and now since the function is "being destroyed" the function needs to destroy its reference to the GFile. @@ +894,3 @@ +gjs_coverage_write_statistics(GjsCoverage *coverage, + GFile *output_file, + gboolean accumulate_coverage) Hm, I'm not 100% sure for the need for both these options, enough that I might suggest that the initial feature land without them. @@ +988,3 @@ + /* This isn't a directory and doesn't have children */ + if (!enumerator) + return; When could this validly happen? I think we should probably record the error we get back from enumerate_children and emit it to the user as a warning -- silently suppressing invalid/bad paths the user passed in is probably a bad idea.
Review of attachment 265498 [details] [review]: ::: gjs/context.cpp @@ +731,2 @@ js_context->hooks = GJS_DEBUG_HOOKS_INTERFACE(gjs_multiplexed_debug_hooks_new(js_context)); + js_context->profiler = gjs_profiler_new(js_context->hooks); Seems weird to me that you went through the effort to make the GjsContext a GjsDebugHooks, but pass it the internal hooks directly. Maybe having a gjs_context_get_debug_hooks(); is a better approach. ::: gjs/profiler.cpp @@ +113,3 @@ self->key.lineno = key->lineno; // Pass ownership of function_name from key to the new function + self->key.function_name = g_strdup (key->function_name); Remove the comment if we're not passing ownership anymore.
Review of attachment 265497 [details] [review]: Not a full review. I tried, but I'm getting really tired and it's 2:30 AM and I figure I'd give you the feedback I have. ::: gjs/context.cpp @@ +96,3 @@ + G_TYPE_OBJECT, + G_IMPLEMENT_INTERFACE(GJS_TYPE_DEBUG_HOOKS_INTERFACE, + gjs_context_debug_hooks_interface_init)) Is there any reason we don't just do gjs_context_get_debug_hooks? This seems a bit superfluous to me. ::: gjs/debug-connection.cpp @@ +1,2 @@ +/* + * Copyright © 2013 Endless Mobile, Inc. Really not a fan of GjsDebugConnection, as we already talked about. I know you hate int handles like gsignal, but I think that's what I'd prefer here. ::: gjs/debug-hooks-private.h @@ +18,3 @@ + * Authored By: Sam Spilsbury <sam.spilsbury@canonical.com> + */ +#ifndef GJS_INTERRUPT_REGISTER_PRIVATE_H Still has the old name. @@ +27,3 @@ +typedef struct _GjsInterruptInfo GjsInterruptInfo; +typedef struct _GjsReflectedScript GjsReflectedScript; +typedef enum _GjsFrameState GjsFrameState; These typedefs are already in debug-hooks.h. Can't you just include that in this file and remove these typedefs? @@ +32,3 @@ + char *filename; + unsigned int line; + char *function_name; Any reason this struct isn't in the "public" interface, and we need the getters? It's not part of the stable ABI here, so we don't have to be too careful. ::: gjs/debug-hooks.h @@ +41,3 @@ + +typedef struct _GjsDebugScriptInfo GjsDebugScriptInfo; +typedef struct _GjsInterruptInfo GjsInterruptInfo; I really don't like the name "GjsInterruptInfo". Could we rename "GjsInterruptInfo" to "GjsFrameInfo" and simply pass "GjsFrameState" as one of the parameters in the callback? @@ +46,3 @@ +typedef enum _GjsFrameState { + GJS_INTERRUPT_FRAME_BEFORE = 0, + GJS_INTERRUPT_FRAME_AFTER = 1 This should be GJS_FRAME_STATE_BEFORE / GJS_FRAME_STATE_AFTER, I assume. Documentation on what this means would be good. @@ +86,3 @@ + GjsInterruptCallback callback, + gpointer user_data); + GjsDebugConnection * (*connect_to_script_load) (GjsDebugHooks *hooks, add_script_load_hook maybe? @@ +89,3 @@ + GjsInfoCallback callback, + gpointer user_data); + GjsDebugConnection * (*connect_to_function_calls_and_execution) (GjsDebugHooks *hooks, This is sort of an unwieldy name. add_execution_hook is the best I can think of for a replacement, though. ::: gjs/multiplexed-debug-hooks.cpp @@ +37,3 @@ +struct _GjsMultiplexedDebugHooksPrivate { + /* Hook lock count */ + unsigned int debug_mode_lock_count; I'm not sure I like the name "lock" for this. It's basically a counter of how many "things" we have using this mode. I can't think of a better one. Maybe "use_count"? @@ +46,3 @@ + * whenever our internal JS debugger hooks get called */ + GHashTable *breakpoints; + GHashTable *pending_breakpoints; Explain the difference between breakpoints and pending_breakpoints? @@ +68,3 @@ + + /* Observing Reference */ + GjsContext *context; Again, "observing reference" isn't standard terminology. context seems like it should also go at the top of the struct, as it's "core data". @@ +134,3 @@ + +static unsigned int +gjs_debug_script_lookup_info_hash (gconstpointer key) Be careful about these spaces. I know it's tricky since gjs doesn't have the space. @@ +152,3 @@ + +static void +gjs_debug_script_lookup_info_destroy (gpointer info) More bad whitespace. @@ +187,3 @@ + +static void +dispatch_interrupt_callbacks (gpointer element, We're only dispatching one callback in this function, so this needs to be "dispatch_interrupt_callback". @@ +292,3 @@ + + if (js_function) + js_function_name = JS_GetFunctionId(js_function); Hm. Because of the heuristics after parsing, this might result in a different value than what the AST will give us. This will mean that we will always need the line-number fallback, rather than just sometimes. (Actually, see the review in a follow-up comment, about function name heuristics being incomplete in general. Maybe this inconsistency doesn't matter. I think it might warrant a test, though) @@ +618,3 @@ + g_hash_table_foreach(priv->pending_breakpoints, + activate_breakpoint_if_within_script, + &activation_data); Considering how many locals we're passing above in what's effectively a closure, this would probably read a lot better as a GHashTableIter: GHashTableIter iter; g_hash_table_iter_init(&iter, priv->pending_breakpoints); while (g_hash_table_iter_next(&iter, &key, &value)) { if (pending_breakpoint_within_script(...)) { activate_pending_breakpoint(...); g_hash_table_remove(priv->pending_breakpoints, key); } } @@ +833,3 @@ + }; + + lock_and_perform_if_unlocked(priv->context, OK, after looking at this, I really don't like how this is structured. It's a bunch of confusing helper functions and genericism that adds more lines of code than I feel is necessary. What's wrong with: if (priv->interrupt_function_use_count == 0) { JS_SetInterrupt(JS_GetRuntime(priv->context), gjs_multiplexed_debug_hooks_interrupt_callback, hooks); } priv->interrupt_function_use_count++; ... priv->interrupt_function_use_count--; if (priv->interrupt_function_use_count == 0) { JS_SetInterrupt(JS_GetRuntime(context), NULL, NULL); } This feels a lot simpler and more readable to me, as it's a lot more concise. (Also, somewhat related: I think we should remove the word "interrupt" from our API. It's confusing since we also have "breakpoint"s and also hardware interrupts. The GjsInterruptInfo struct seems to describe information about a stack frame, too.) @@ +1100,3 @@ + +static JSScript * +lookup_script_for_filename_with_closest_baseline_floor(GjsMultiplexedDebugHooks *hooks, What's a "baseline floor"? ::: gjs/reflected-executable-script.cpp @@ +526,3 @@ { + GjsReflectedExecutableScript *hooks = GJS_REFLECTED_EXECUTABLE_SCRIPT(object); + GjsReflectedExecutableScriptPrivate *priv = (GjsReflectedExecutableScriptPrivate *) gjs_reflected_executable_script_get_instance_private(hooks); Excuse me? What's the point of this rename?
Hey Jasper, Thanks for the review! Most of the style nits here are fine for me to fix (its just hard to find them all when one has been looking at the same code for days, also, renaming things is annoying, heh). I'm only responding to the areas that I think warrant discussion.
(In reply to comment #31) > Review of attachment 265497 [details] [review]: > > Not a full review. I tried, but I'm getting really tired and it's 2:30 AM and I > figure I'd give you the feedback I have. > > ::: gjs/context.cpp > @@ +96,3 @@ > + G_TYPE_OBJECT, > + G_IMPLEMENT_INTERFACE(GJS_TYPE_DEBUG_HOOKS_INTERFACE, > + > gjs_context_debug_hooks_interface_init)) > > Is there any reason we don't just do gjs_context_get_debug_hooks? This seems a > bit superfluous to me. Its a matter of principle I suppose - I'd rather not have the tools need to know about the context internals or have to reach into them in order to use the debugging interface. They just need to know about something which implements that interface. > > ::: gjs/debug-connection.cpp > @@ +1,2 @@ > +/* > + * Copyright © 2013 Endless Mobile, Inc. > > Really not a fan of GjsDebugConnection, as we already talked about. I know you > hate int handles like gsignal, but I think that's what I'd prefer here. The problem with int handles is that unnecessary coupling is forced between the disconnection action and the thing which is being disconnected from at disconnection time. For instance, a tool could just have a GjsDebugHooks at construct time, connect to some callbacks and then forget about it later. The client in this instance doesn't care about the GjsDebugHooks instance, it just wants to disconnect the callbacks. What was the reason for not liking GjsDebugConnection? > > ::: gjs/debug-hooks-private.h > @@ +18,3 @@ > + * Authored By: Sam Spilsbury <sam.spilsbury@canonical.com> > + */ > +#ifndef GJS_INTERRUPT_REGISTER_PRIVATE_H > > Still has the old name. > > @@ +27,3 @@ > +typedef struct _GjsInterruptInfo GjsInterruptInfo; > +typedef struct _GjsReflectedScript GjsReflectedScript; > +typedef enum _GjsFrameState GjsFrameState; > > These typedefs are already in debug-hooks.h. Can't you just include that in > this file and remove these typedefs? I prefer to minimize include dependencies as much as possible, especially in header files. Though technically speaking, multiple-typedef-to-the-same-thing is technically a gcc extension and I don't know if we care about clang or not. Do we? > > @@ +32,3 @@ > + char *filename; > + unsigned int line; > + char *function_name; > > Any reason this struct isn't in the "public" interface, and we need the > getters? It's not part of the stable ABI here, so we don't have to be too > careful. Yes - not having it public means that we can enforce const field constness on the client side without having to do weird casting on the implementation side. Though I know const is mostly an empty promise in C/GObject land. > > ::: gjs/debug-hooks.h > @@ +41,3 @@ > + > +typedef struct _GjsDebugScriptInfo GjsDebugScriptInfo; > +typedef struct _GjsInterruptInfo GjsInterruptInfo; > > I really don't like the name "GjsInterruptInfo". Could we rename > "GjsInterruptInfo" to "GjsFrameInfo" and simply pass "GjsFrameState" as one of > the parameters in the callback? I guess so (considering the discussion of "Interrupt" below). > > @@ +46,3 @@ > +typedef enum _GjsFrameState { > + GJS_INTERRUPT_FRAME_BEFORE = 0, > + GJS_INTERRUPT_FRAME_AFTER = 1 > > This should be GJS_FRAME_STATE_BEFORE / GJS_FRAME_STATE_AFTER, I assume. > > Documentation on what this means would be good. > > @@ +86,3 @@ > + GjsInterruptCallback callback, > + gpointer user_data); > + GjsDebugConnection * (*connect_to_script_load) (GjsDebugHooks *hooks, > > add_script_load_hook maybe? I try to pick names in interfaces which are meaningful for the client of the interface as opposed to ones that describe an internal process of some implementation. The client wants to connect to script loads (and it receives a connection). Though maybe add_script_load_hook also works. I'll try it and see how it fits. > > @@ +89,3 @@ > + GjsInfoCallback callback, > + gpointer > user_data); > + GjsDebugConnection * (*connect_to_function_calls_and_execution) > (GjsDebugHooks *hooks, > > This is sort of an unwieldy name. add_execution_hook is the best I can think of > for a replacement, though. Yeah, I guess so, although "execution" is a bit vague. > @@ +292,3 @@ > + > + if (js_function) > + js_function_name = JS_GetFunctionId(js_function); > > Hm. Because of the heuristics after parsing, this might result in a different > value than what the AST will give us. This will mean that we will always need > the line-number fallback, rather than just sometimes. > > (Actually, see the review in a follow-up comment, about function name > heuristics being incomplete in general. Maybe this inconsistency doesn't > matter. I think it might warrant a test, though) > Review of attachment 265499 [details] [review]: > > ::: gjs/coverage.cpp > @@ +272,3 @@ > + g_hash_table_replace(file_statistics->functions, > + g_strdup(function_name), > + GINT_TO_POINTER(hit_count)); > > Yikes. There's no guarantee that two functions won't have the same name: > > function foo() { > return (function test() { > return "foo"; > }); > } > > function bar() { > return (function test() { > return "bar"; > }); > } > > foo(); > bar(); > > As I understand it, this will emit FN:test:2, even though each test(); got > called once. It seems the format we emit coverage reports in doesn't really > have anything better, so I'm wondering if it makes sense to have function name > reporting at all. Maybe this is such an edge case that it doesn't really > matter. Maybe it makes sense to emit a warning in the results if we have the > same function name recorded for two different line numbers. > > Hm... what's the lesser of the two evils: potentially inaccurate data, or no > data at all? > > There should probably be a test case for this behavior. So lets talk about this here: After seeing some of your other comments on this, I think it would be good to either: 1. Have info_reflect.js only return names like function:lineno (maybe function:lineno:colno to handle the function () { return function() {} } case, though I don't know how we would be able to detect this in the execution hook). 2. Have it return an array of { function_name: (if available) position: AST position (see comments above about column numbers. } Worst case we'll just have to have it warn about functions which are overly similar if we can't differentiate on column boundaries. The coverage tool just doesn't care about function names that much - it only wants to know how many there are and a unique ID for each. > > @@ +833,3 @@ > + }; > + > + lock_and_perform_if_unlocked(priv->context, > > OK, after looking at this, I really don't like how this is structured. It's a > bunch of confusing helper functions and genericism that adds more lines of code > than I feel is necessary. What's wrong with: > > if (priv->interrupt_function_use_count == 0) { > JS_SetInterrupt(JS_GetRuntime(priv->context), > gjs_multiplexed_debug_hooks_interrupt_callback, > hooks); > } > priv->interrupt_function_use_count++; > > ... > > priv->interrupt_function_use_count--; > if (priv->interrupt_function_use_count == 0) { > JS_SetInterrupt(JS_GetRuntime(context), NULL, NULL); > } > > This feels a lot simpler and more readable to me, as it's a lot more concise. I suppose so, although I guess a design goal of the original interface here was to ensure that we always use the counters in the same way. I guess that's not a huge issue though. You can probably tell that I miss templates a lot here ;-) I definitely would like if (priv->interrupt_function_use_count == 0) { JS_SetInterrupt(JS_GetRuntime(priv->context), gjs_multiplexed_debug_hooks_interrupt_callback, hooks); } priv->interrupt_function_use_count++; ... priv->interrupt_function_use_count--; if (priv->interrupt_function_use_count == 0) { JS_SetInterrupt(JS_GetRuntime(context), NULL, NULL); } To live in a separate function. Getting these counters right can be error prone and we want to enforce coupling between them and the action at hand. I think we're in agreement about that though. > > @@ +283,3 @@ > + * A value of -1 for an element means that the line is non-executable and > never actually > + * reached. A value of 0 means that it was executable but never reached. A > positive value > + * indicates the hit count. > > Should probably mention the (obvious) reason for actually caring about > non-executable vs. executable lines: we only care about coverage for executable > lines, and so we don't want to report coverage misses for non-executable lines. > > @@ +291,3 @@ > + * case we can neatly handle the error by marking that line executable. A > hit on a line > + * we thought was non-executable is not as much of a problem as noise > generated by > + * ostensible "misses" which could in fact never be executed. > > Just curious: are there any known cases where we'll receive data for a supposed > non-executable line? Could we have a test case? I had it a few times when creating the find-lines-by-reflection thing. This is more of a thing to handle edge cases where it turns out that info_reflect got it wrong for whatever reason. Defensive coding. Maybe we can warn about it now. I've run the reflection tool over a few different codebases and was able to find all the executable lines there, but I'm sure there's some edge cases around. I'd rather not enumerate every hypothetical one for the sake of saving time. > > @@ +301,3 @@ > + > + /* We are ignoring the zeroth line, so we want line_count + 1 */ > + g_array_set_size(line_statistics, line_count + 1); > > I think you can use g_array_new_sized as a shortcut here. It only increases the array allocation size, not the array length (which are two, slightly different concepts). Though I think an abstraction is warranted here. > No need to comment on refcounting. > > Is there ever a reason where you *wouldn't* be at the end of the stream if it's > one we've kept? Did I miss a seek somewhere, or is it a sort of "just in case" > thing? I think its a just-in-case thing. I can remove the seek and see if the tests still pass. > > @@ +791,3 @@ > + /* Unreferencing the tracefile is safe here as the > + * underlying GFileOutputStream does not have > + * an observing reference to it */ > > The comment here is only confusing. This is fairly standard GObject: this > function "owns" the object GFile it created it, we pass it to ostream which > takes a reference (since it needs it for itself), and now since the function is > "being destroyed" the function needs to destroy its reference to the GFile. Ah okay, I thought the convention might have been different. Removing. > > @@ +894,3 @@ > +gjs_coverage_write_statistics(GjsCoverage *coverage, > + GFile *output_file, > + gboolean accumulate_coverage) > > Hm, I'm not 100% sure for the need for both these options, enough that I might > suggest that the initial feature land without them. Output File and Accumulate Coverage? Yeah, they're kind of important :) Output File: You don't really want *.js.info everywhere in the source tree do you? Accumulate Coverage: We'll want to hook this up to make check and Automake insists upon knowing about each individual test script. On each run of a test script, coverage data would normally be reset, although with --accumulate we can just append-to instead (genhtml handles this fine, in fact, this is the way it is suppose to be used if you want accumulation). Yeah, its more of a workaround for Automake suckyness, but nonetheless, still a valid option. > > @@ +988,3 @@ > + /* This isn't a directory and doesn't have children */ > + if (!enumerator) > + return; > > When could this validly happen? I think we should probably record the error we > get back from enumerate_children and emit it to the user as a warning -- > silently suppressing invalid/bad paths the user passed in is probably a bad > idea. +1, lets do that instead.
(In reply to comment #34) > Its a matter of principle I suppose - I'd rather not have the tools need to > know about the context internals or have to reach into them in order to use the > debugging interface. They just need to know about something which implements > that interface. I suppose it's just a matter of code style. We tend not to use interfaces in GObject code unless we really want polymorphism. I don't think saying "connect to these hooks" is context internals. > The problem with int handles is that unnecessary coupling is forced between the > disconnection action and the thing which is being disconnected from at > disconnection time. For instance, a tool could just have a GjsDebugHooks at > construct time, connect to some callbacks and then forget about it later. The > client in this instance doesn't care about the GjsDebugHooks instance, it just > wants to disconnect the callbacks. It's not unnecessary, I'd say, and anywhere where we don't have a GjsContext is really bad -- GjsContext is really needed to do literally anything with the GJS API, so... > What was the reason for not liking GjsDebugConnection? It's just non-standard for GObject code. I'd prefer if we used more of the platform than inventing our own weird stuff. There's also GHookList and GClosure you could look at (I haven't seen GHookList used before, desrt told me about it on IRC, it's probably some weird API we don't recommend), but I think a standard: gjs_debug_hooks_add_xxx_callback(hooks, callback, user_data); gjs_debug_hooks_remove_xxx_callback(hooks, callback, user_data); should work fine. No need for int handles. > I prefer to minimize include dependencies as much as possible, especially in > header files. > > Though technically speaking, multiple-typedef-to-the-same-thing is technically > a gcc extension and I don't know if we care about clang or not. Do we? clang tries to support all of the sane gcc extensions -- it's considered a bug in clang if it's not supported. I don't think it does, though. We usually make the private API depend on the public API. > Yes - not having it public means that we can enforce const field constness on > the client side without having to do weird casting on the implementation side. > > Though I know const is mostly an empty promise in C/GObject land. What does a const pointer in a struct even tell us? I don't think it matters. > I try to pick names in interfaces which are meaningful for the client of the > interface as opposed to ones that describe an internal process of some > implementation. The client wants to connect to script loads (and it receives a > connection). > > Though maybe add_script_load_hook also works. I'll try it and see how it fits. Yeah, I don't really like "connect" for a client API. It just sounds weird to me for some reason. > Yeah, I guess so, although "execution" is a bit vague. I tried to think of something better, but I couldn't. Documentation on what this does exactly would help a lot. > So lets talk about this here: > > ... > > Worst case we'll just have to have it warn about functions which are overly > similar if we can't differentiate on column boundaries. > The coverage tool just doesn't care about function names that much - it only > wants to know how many there are and a unique ID for each. So, may I ask why exactly we care about functions? What does the report output look like and why? I'm not exactly sure what the coverage reporting is doing here... > I suppose so, although I guess a design goal of the original interface here was > to ensure that we always use the counters in the same way. I guess that's not a > huge issue though. You can probably tell that I miss templates a lot here ;-) > > I definitely would like > > if (priv->interrupt_function_use_count == 0) { > JS_SetInterrupt(JS_GetRuntime(priv->context), > gjs_multiplexed_debug_hooks_interrupt_callback, > hooks); > } > priv->interrupt_function_use_count++; > > ... > > priv->interrupt_function_use_count--; > if (priv->interrupt_function_use_count == 0) { > JS_SetInterrupt(JS_GetRuntime(context), NULL, NULL); > } > > To live in a separate function. Getting these counters right can be error prone > and we want to enforce coupling between them and the action at hand. I think > we're in agreement about that though. Oh, of course. Reading back on that, I see it's very clear. Keep the lock_blah(); and unlock_blah(); functions separate, just remove the lock_things_and_maybe_do_things_if_unlocked(); genericism. > I had it a few times when creating the find-lines-by-reflection thing. > > This is more of a thing to handle edge cases where it turns out that > info_reflect got it wrong for whatever reason. Defensive coding. Maybe we can > warn about it now. > > I've run the reflection tool over a few different codebases and was able to > find all the executable lines there, but I'm sure there's some edge cases > around. I'd rather not enumerate every hypothetical one for the sake of saving > time. Yeah, I was just curious if it was something you actually hit, or if it was a hypothetical. A warning when we hit it would be nice, simply because I think it might warrant investigation. > It only increases the array allocation size, not the array length (which are > two, slightly different concepts). Though I think an abstraction is warranted > here. Yikes. That's some bad API on our part, considering how "new_sized" and "set_size" are both quite alike. > Output File and Accumulate Coverage? Yeah, they're kind of important :) > > Output File: You don't really want *.js.info everywhere in the source tree do > you? If we don't really want that, why do we have code to do it? > Accumulate Coverage: We'll want to hook this up to make check and Automake > insists upon knowing about each individual test script. On each run of a test > script, coverage data would normally be reset, although with --accumulate we > can just append-to instead (genhtml handles this fine, in fact, this is the way > it is suppose to be used if you want accumulation). > > Yeah, its more of a workaround for Automake suckyness, but nonetheless, still a > valid option. I think we should really only strive to support one or two supported configurations, rather than a 2x2 matrix. Do the right thing by default. If putting it in some output dir and accumulating is the best way to do it, then hardcode some default directory for output, and let the user/automake write `rm -rf` if they don't want accumulation. Yes, my GNOME "no settings" colors are showing through here :)
Created attachment 265931 [details] [review] context: Move the majority of gjs_context_eval to jsapi-util And use it in the importer as well...
Created attachment 265932 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 265934 [details] [review] Remove unused gjs_files_with_tests
Created attachment 265935 [details] [review] Fix context leak.
Created attachment 265937 [details] [review] Fix program name leak (1 byte).
Created attachment 265938 [details] [review] Use g_file_new_for_commandline_arg in gjs_context_eval_file g_file_get_contents uses open() directly, but we really want to create a file using GVFS so that we can evaluate files which are actually resource URIs.
Created attachment 265939 [details] [review] Add GjsReflectedScript and GjsReflectedExecutableScript GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines. GjsReflectedExecutableScript is an implementation of the former which can be updated as-needs-be at runtime (while new executable lines are added).
Review of attachment 265932 [details] [review]: The strip_unix_shebang API isn't great. Perhaps instead of taking a silly in/out pointer, we just forcefully set it to 1 or 2. ::: gjs/jsapi-util.cpp @@ +1171,3 @@ +gjs_strip_unix_shebang(const char **script, + gssize *script_len, + int *line_number) You should fix it in both the header and the C declaration.
Review of attachment 265934 [details] [review]: OK. Feel free to rebase to the top of the stack and push immediately.
Review of attachment 265935 [details] [review]: Given your troubles with valgrind, and given how we'll still leak in the error case, I'm not sure about this. Fix the error case to not leak as well and I'll be a lot happier.
Review of attachment 265937 [details] [review]: OK. I'm not sure what the "1 byte" thing is about, and again, we don't put periods at the end of commit messages. But OK.
Review of attachment 265938 [details] [review]: ::: gjs/context.cpp @@ +883,3 @@ + if (!g_file_query_exists(file, NULL)) { + g_object_unref(file); + return NULL; Nope. It returns bool. And again, I'd prefer a "goto out;" pattern here so we didn't have to free things ten times over. It's easy to miss something.
(In reply to comment #47) > Review of attachment 265938 [details] [review]: > > ::: gjs/context.cpp > @@ +883,3 @@ > + if (!g_file_query_exists(file, NULL)) { > + g_object_unref(file); > + return NULL; > > Nope. It returns bool. And again, I'd prefer a "goto out;" pattern here so we > didn't have to free things ten times over. It's easy to miss something. ... Why did my compiler not warn about this one :/ Ugh. Goto is really not a good idea in C++ because it can cause destructors to be skipped. Maybe in this case it is harmless.
Review of attachment 265931 [details] [review]: Reviewing my own patch would be cheating, of course. I'll let Giovanni or Colin do this one.
(In reply to comment #48) > ... Why did my compiler not warn about this one :/ > > Ugh. Goto is really not a good idea in C++ because it can cause destructors to > be skipped. Maybe in this case it is harmless. We use this pattern a lot, and I don't think there's any cases where we "goto out;" of a block containing an RAII variable (though it would be really good to verify this!). Perhaps libgsystem and gs_unref_object are the way to go.
(In reply to comment #50) > (In reply to comment #48) > > ... Why did my compiler not warn about this one :/ > > > > Ugh. Goto is really not a good idea in C++ because it can cause destructors to > > be skipped. Maybe in this case it is harmless. > > We use this pattern a lot, and I don't think there's any cases where we "goto > out;" of a block containing an RAII variable (though it would be really good to > verify this!). > > Perhaps libgsystem and gs_unref_object are the way to go. Sounds like a good idea, although pulling in another submodule seems out of scope for this patch set. Its already big enough! I'll look into the goto situation. (Out of curiosity, would there be anything wrong with using real RAII? Maybe the Anti-C++ brigade will come to get me, heh)
(In reply to comment #51) > Sounds like a good idea, although pulling in another submodule seems out of > scope for this patch set. Its already big enough! I'll look into the goto > situation. > > (Out of curiosity, would there be anything wrong with using real RAII? Maybe > the Anti-C++ brigade will come to get me, heh) Nothing, it's just that we don't have a set of RAII wrappers for GObject and co. but we do have a set of gcc destructor wrappers that are the exact same thing as RAII. Just less code to write and understand for GNOME people. We're lazy :)
Review of attachment 265931 [details] [review]: Could use a rationale, like "To deduplicate evaluation code"? ::: gjs/jsapi-util.cpp @@ +1177,3 @@ + const char *s; + + s = (const char *) strstr (*script, "\n"); strchr '\n'
Created attachment 265978 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 265979 [details] [review] Fix context leak
Created attachment 265980 [details] [review] Use g_file_new_for_commandline_arg in gjs_context_eval_file g_file_get_contents uses open() directly, but we really want to create a file using GVFS so that we can evaluate files which are actually resource URIs.
Created attachment 265982 [details] [review] Add GjsReflectedScript and GjsReflectedExecutableScript GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines. GjsReflectedExecutableScript is an implementation of the former which can be updated as-needs-be at runtime (while new executable lines are added).
Review of attachment 265978 [details] [review]: ::: gjs/jsapi-util.cpp @@ +1171,3 @@ +gjs_strip_unix_shebang(const char *script, + gssize script_len, + gssize *script_len_out, Seems weird to have both an in and an out pointer, but not gonna hark on it. @@ +1183,3 @@ + + if (start_line_number_out) + *start_line_number_out = 2; Won't this get stomped by the assignment below?
Review of attachment 265979 [details] [review]: OK. Would like a "goto out;" here, but not gonna complain too loudly.
Created attachment 265983 [details] [review] Add GjsDebugHooks interface and GjsMultiplexedDebugHooks object This interface (and object) serve as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsDebugHooks, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a guint. Remove the debugging hook with the relevant _remove function.
Review of attachment 265980 [details] [review]: ::: gjs/context.cpp @@ +878,3 @@ + char *script = NULL; + gsize script_len; + gboolean retval = TRUE; Usually, in the gjs style, the boolean is called "ret" and the returned jsval is called "retval". @@ +900,2 @@ g_free(script); +out: Why the two jumps here?
Created attachment 265984 [details] [review] Integrate GjsProfiler with GjsInterruptRegister GjsProfiler now takes a strong reference to GjsDebugHooks and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Created attachment 265985 [details] [review] Added GjsCoverage object (and thus code coverage support) This object effectively just takes a connection to the single step handler and records how many times we hit each line. To prevent clutter, files considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a file to generate coverage reports for. If paths are specified, then --coverage-output is mandatory in order to specify a directory to write coverage reports to. In order to handle writing reports for URIs that GVfs might understand but lcov would not, we need to copy the covered files into this directory. Coverage reports are append-only by default. Just remove the report if you need to zero the counters.
(In reply to comment #58) > Review of attachment 265978 [details] [review]: > > ::: gjs/jsapi-util.cpp > @@ +1171,3 @@ > +gjs_strip_unix_shebang(const char *script, > + gssize script_len, > + gssize *script_len_out, > > Seems weird to have both an in and an out pointer, but not gonna hark on it. Yeah it is a bit. I think I wanted to make the outvals optional. Actually this function is really weird, but I can't think of any better ways to do it. Any ideas? > > @@ +1183,3 @@ > + > + if (start_line_number_out) > + *start_line_number_out = 2; > > Won't this get stomped by the assignment below? Nice catch, fixed by an early return.
(In reply to comment #53) > Review of attachment 265931 [details] [review]: > > Could use a rationale, like "To deduplicate evaluation code"? > > ::: gjs/jsapi-util.cpp > @@ +1177,3 @@ > + const char *s; > + > + s = (const char *) strstr (*script, "\n"); > > strchr '\n' Fixed in attached patch.(In reply to comment #61) > Review of attachment 265980 [details] [review]: > > ::: gjs/context.cpp > @@ +878,3 @@ > + char *script = NULL; > + gsize script_len; > + gboolean retval = TRUE; > > Usually, in the gjs style, the boolean is called "ret" and the returned jsval > is called "retval". Thanks, will fix. > > @@ +900,2 @@ > g_free(script); > +out: > > Why the two jumps here? Isn't that usually the convention when using goto for cleanup? Thing *thing = make_thing(); if (!thing) goto out_thing; OtherThing *other = make_other_thing(); if (!other) goto out_other; .... out_other: destroy_other(other); out_thing: destroy_thing(thing); As an aside, the way I usually handle during functions which need to construct things with multiple dependencies is to have a separate cleanup function, e.g.: gboolean cleanup_thing(Thing *thing) { destroy_thing(thing); return FALSE; gboolean cleanup_other_thing(Thing *thing, Other *other) { cleanup_thing(thing); destroy_other(other); return FALSE; } gboolean do_thing() { Thing *thing = make_thing(); if (!thing) return cleanup_do_thing(thing); Other *other = make_other_thing_depending_on_thing(thing); if (!other) return cleanup_other_thing(thing, other); cleanu_other_thing(thing, other); return TRUE; } Its lispy but one alternative for the (unfortunate) lack of RAII in case there was any interest.
Created attachment 266011 [details] [review] context: Move the majority of gjs_context_eval to jsapi-util And use it in the importer as well. This deduplicates a large part of the script evaluation code.
Created attachment 266012 [details] [review] Remove unused gjs_files_with_tests
Created attachment 266013 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 266014 [details] [review] Fix context leak
Created attachment 266015 [details] [review] Use g_file_new_for_commandline_arg in gjs_context_eval_file g_file_get_contents uses open() directly, but we really want to create a file using GVFS so that we can evaluate files which are actually resource URIs.
Created attachment 266016 [details] [review] Add GjsReflectedScript and GjsReflectedExecutableScript GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines. GjsReflectedExecutableScript is an implementation of the former which can be updated as-needs-be at runtime (while new executable lines are added).
Created attachment 266017 [details] [review] Add GjsDebugHooks interface and GjsMultiplexedDebugHooks object This interface (and object) serve as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsDebugHooks, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a guint. Remove the debugging hook with the relevant _remove function.
Created attachment 266018 [details] [review] Integrate GjsProfiler with GjsInterruptRegister GjsProfiler now takes a strong reference to GjsDebugHooks and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Created attachment 266019 [details] [review] Added GjsCoverage object (and thus code coverage support) This object effectively just takes a connection to the single step handler and records how many times we hit each line. To prevent clutter, files considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a file to generate coverage reports for. If paths are specified, then --coverage-output is mandatory in order to specify a directory to write coverage reports to. In order to handle writing reports for URIs that GVfs might understand but lcov would not, we need to copy the covered files into this directory. Coverage reports are append-only by default. Just remove the report if you need to zero the counters.
Created attachment 266020 [details] [review] Generate coverage reports for Gjs Added the necessary Makefile bits to generate coverage reports using gcov and also hook up gjs-unit to our own internal coverage generator and generate coverage reports for our module scripts. "make lcov" can be used to generate coverage reports.
(In reply to comment #65) > Isn't that usually the convention when using goto for cleanup? We tend to just have one "out" label, rather than multiple. free/g_free thankfully handles NULL pointers just fine, so you don't need multiple labels in this case.
Review of attachment 266014 [details] [review]: ::: gjs/console.cpp @@ +116,3 @@ argc - 1, (const char**)argv + 1, &error)) { g_printerr("Failed to defined ARGV: %s", error->message); Won't we also leak the error here and below? I'm sorry for hitting on this one way too much, but I'd really prefer a "goto out;" here, now that I look at it again.
Review of attachment 266012 [details] [review]: As said before, please push immediately... wait, you do have a GNOME git account, right?
Review of attachment 266011 [details] [review]: OK.
Review of attachment 266013 [details] [review]: ::: gjs/jsapi-util.cpp @@ +1169,3 @@ +const char * +gjs_strip_unix_shebang(const char *script, I think I much prefer the inout parameters for this API. @@ +1191,3 @@ + * that script_len_out and start_line_number_out + * were unchanged */ + return NULL; What? Won't the code below crash on this case, then? ::: gjs/jsapi-util.h @@ +392,3 @@ GError **error); +/* Returns a pointer to the beginning of a script with unix We usually use traditional doc comments, which means above the function in the C function, and of the form: /** * gjs_eval_with_scope: * ... */
Review of attachment 266015 [details] [review]: ::: gjs/context.cpp @@ +897,3 @@ } +out_script: Already talked about this.
Review of attachment 266016 [details] [review]: ::: gjs/reflected-executable-script.cpp @@ +189,3 @@ + +static GArray * +call_js_function_for_array_return(JSContext *context, You didn't take my earlier review comments about this into effect. @@ +273,3 @@ + * instance. + * + * See https://bugzilla.gnome.org/show_bug.cgi?id=721678 */ I don't think a comment here is really appropriate -- we assume this everywhere we use gjs_string_to_utf8, which is all over the place. @@ +576,3 @@ + * need to explicitly clear the current context and restore it later */ +static GjsContext * +push_new_context() Again, push_new_context(void). And I'd still like to see a context stack supported explicitly by the context.cpp code. We should probably also remove the list_all_contexts API. ::: gjs/reflected-script.cpp @@ +23,3 @@ +static void gjs_reflected_script_interface_default_init(GjsReflectedScriptInterface *settings_interface); + +G_DEFINE_INTERFACE(GjsReflectedScript, gjs_reflected_script_interface, G_TYPE_OBJECT); Still gonna be a bit picky about the unnecessary use of interfaces -- we tend to only use interfaces when we have multiple implementers. ::: modules/infoReflect.js @@ +198,3 @@ + * to the first line where execution starts */ +function getBranchExitStartLine(branchBodyNode) { + Unnecessary whitespace. @@ +268,3 @@ + * so find the actual exits */ + for (let i = 0; i < node.cases.length; i++) + branchExitNodes.push(node.cases[i]); This can just be Array.prototype.push.apply(branchExitNodes, node.cases);, right? @@ +275,3 @@ + + let branchExitStartLines = branchExitNodes.map(function(exitNode) { + return getBranchExitStartLine(exitNode); This can just be branchExitNodes.map(getBranchExitStartLine); @@ +278,3 @@ + }); + + branchExitStartLines = branchExitStartLines.filter(function(value, index, array) { I tend not to include the extra params when they're unused. Also, you should probably name the parameter "line" instead of "value". @@ +338,3 @@ +function expressionLinesForAST(ast) { + let allExpressions = collectForSubNodes(ast.body, expressionLinesForNode); + /* Deduplicate the list */ No need for this comment.
Review of attachment 266017 [details] [review]: Most of the last review still applies, such as GjsInterruptInfo. I'm happier now that GjsDebugCallback is gone, though. ::: gjs/debug-hooks.cpp @@ +23,3 @@ +static void gjs_debug_hooks_interface_default_init(GjsDebugHooksInterface *settings_interface); + +G_DEFINE_INTERFACE(GjsDebugHooks, gjs_debug_hooks_interface, G_TYPE_OBJECT); Now if only I can convince you to get rid of this interface :) Is there any reason for more than one implementation to exist? ::: gjs/debug-hooks.h @@ +40,3 @@ + +typedef struct _GjsDebugScriptInfo GjsDebugScriptInfo; +typedef struct _GjsInterruptInfo GjsInterruptInfo; Still contains this.
Review of attachment 266018 [details] [review]: Commit message still needs updating, as it says "GjsInterruptRegister". "Strong reference" in commit message needs removing, etc. ::: gjs/profiler.h @@ +34,1 @@ +GjsProfiler *gjs_profiler_new (GjsDebugHooks *interrupts); I still think we should just take the GjsContext here.
Gonna wait on the last two reviews before you fix most of the review comments from previous patches.
Created attachment 266200 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 266201 [details] [review] Fix context leak
Created attachment 266202 [details] [review] Use g_file_new_for_commandline_arg in gjs_context_eval_file g_file_get_contents uses open() directly, but we really want to create a file using GVFS so that we can evaluate files which are actually resource URIs.
Created attachment 266203 [details] [review] Add a context stack API The current context is now based on a stack model, so clients can freely push and pop contexts. The current context is the top of the stack.
Created attachment 266204 [details] [review] Add GjsReflectedScript GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines.
Created attachment 266205 [details] [review] Add GjsDebugHooks interface This interface (and object) serve as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsDebugHooks, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a guint. Remove the debugging hook with the relevant _remove function.
Created attachment 266206 [details] [review] Integrate GjsProfiler with GjsDebugHooks GjsProfiler now takes a strong reference to GjsDebugHooks and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Created attachment 266207 [details] [review] Added GjsCoverage object (and thus code coverage support) This object effectively just takes a connection to the single step handler and records how many times we hit each line. To prevent clutter, files considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a file to generate coverage reports for. If paths are specified, then --coverage-output is mandatory in order to specify a directory to write coverage reports to. In order to handle writing reports for URIs that GVfs might understand but lcov would not, we need to copy the covered files into this directory. Coverage reports are append-only by default. Just remove the report if you need to zero the counters.
Created attachment 266208 [details] [review] Generate coverage reports for Gjs Added the necessary Makefile bits to generate coverage reports using gcov and also hook up gjs-unit to our own internal coverage generator and generate coverage reports for our module scripts. "make lcov" can be used to generate coverage reports.
Created attachment 266226 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 266227 [details] [review] Add a context stack API The current context is now based on a stack model, so clients can freely push and pop contexts. The current context is the top of the stack.
Created attachment 266247 [details] [review] Added GjsCoverage object (and thus code coverage support) This object effectively just takes a connection to the single step handler and records how many times we hit each line. To prevent clutter, files considered to be part of the coverage report must be specified inclusively, anything not part of those paths will not be included in the coverage report. The coverage tool is integrated in to gjs-console, you can use it by specifying -C or --coverage-path to add a file to generate coverage reports for. If paths are specified, then --coverage-output is mandatory in order to specify a directory to write coverage reports to. In order to handle writing reports for URIs that GVfs might understand but lcov would not, we need to copy the covered files into this directory. Coverage reports are append-only by default. Just remove the report if you need to zero the counters.
Created attachment 266252 [details] [review] Make gjs_strip_unix_shebang public and clarify some variable names line_number in this instance is really the starting line number.
Created attachment 266255 [details] [review] Fix context leak
Created attachment 266256 [details] [review] Use g_file_new_for_commandline_arg in gjs_context_eval_file g_file_get_contents uses open() directly, but we really want to create a file using GVFS so that we can evaluate files which are actually resource URIs.
Created attachment 266257 [details] [review] Add a context stack API The current context is now based on a stack model, so clients can freely push and pop contexts. The current context is the top of the stack.
Created attachment 266258 [details] [review] Add GjsReflectedScript GjsReflectedScript contains some useful information about a script, including an array with all available function names, array of all branches and an array of all executable lines.
Created attachment 266260 [details] [review] Add GjsDebugHooks This object serves as a central point for clients to connect to SpiderMonkey's internal debugging functions without stepping on each other. Through GjsDebugHooks, clients are able to connect to the general execution hooks (such as entry and exit of new functions and source files), enable single-step mode and recieve interrupts and add arbitrary breakpoints on source files. Each of these functions takes a callback and returns a guint. Remove the debugging hook with the relevant _remove function.
Created attachment 266261 [details] [review] Integrate GjsProfiler with GjsDebugHooks GjsProfiler now takes a strong reference to GjsDebugHooks and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Created attachment 266279 [details] [review] Integrate GjsProfiler with GjsDebugHooks GjsProfiler now takes a strong reference to GjsDebugHooks and uses its function calls and execution hook in order to gather profiling info instead of using SpiderMonkey's debugging API directly
Obsoleting a bunch of old patches. If this is wrong, please let me know. If you have a git branch of this anywhere, that would probably be a lot easier for me to look at as well.
Attachment 266011 [details] pushed as d6f8757 - context: Move the majority of gjs_context_eval to jsapi-util
Closing: Support was added for this https://git.gnome.org/browse/gjs/commit/?id=7ef7d25df0fffec34c56ce97fbe1e11f81606673