506717 - GDB Python debugging support for SpiderMonkey should be in tree

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The "Archer" project has added Python-based scripting to GDB on Linux.  Archer's changes are being integrated into the main GDB code base over time.  Among other things, Archer allows you to write custom printing code for the debuggee's types in Python: jsval and jsid types can be de-tagged and printed appropriately; JSString values can print as quoted strings; and so on.

Using appropriately named and loaded files, you can have Archer automatically load the pretty-printers for an executable or shared library.

I've written Archer files for pretty-printing some common SpiderMonkey types, and I've been keeping them in a separate repository:
http://hg.mozilla.org/users/jblandy_mozilla.com/archer-mozilla/

However, it would be more convenient to have them in the Mozilla repository, where Archer can auto-load them, and where all developers with Archer-fu can contribute to them.

Comment 1

[Jim Blandy :jimb]

Attachment: GDB pretty-printing support for SpiderMonkey.

This is a big patch, but the only part that affects the build is the test executable, which is built along with anything else.

The way GDB loads the pretty-printers automatically is by looking for a file called <executable>-gdb.py next to the executable or library you're debugging. At the moment, I only install a GDB auto-load file for the JavaScript shell. After the SpiderMonkey developers have had some time to see if they like it, we can add an auto-load file for Firefox.

Comment 2

[Jim Blandy :jimb]

Attachment: GDB pretty-printing support for SpiderMonkey.

Revised: now with jsid support and transparency for Handles, Roots, HeapSlots, and so on.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 684710 [details] [diff] [review]
GDB pretty-printing support for SpiderMonkey.

Review of attachment 684710 [details] [diff] [review]:
-----------------------------------------------------------------

This much looks beautiful. r=me for the non-build parts. Though this is still missing quite a bit of functionality from your original version.

Do you think it would be ok to build all of the gdb tests as part of either the default tinderbox build or make check? I'd sort of like people to know immediately if the test programs don't even compile anymore. I don't think people are ready to backout if the gdb tests fail, but breaking the compilation shouldn't be too bad.

Actually, we link everything in jsapi-tests in the default build, so maybe we could even add it in there.

I suppose it's better to do something like that in a followup, though.

::: js/src/Makefile.in
@@ +31,4 @@
>  TEST_DIRS += jsapi-tests
>  endif
>  
> +TEST_DIRS += tests gdb

This is going to require a build peer review.

::: js/src/gdb/Makefile.in
@@ +27,5 @@
> +LOCAL_INCLUDES += -I$(topsrcdir) -I..
> +
> +ifeq ($(OS_ARCH),Darwin)
> +ifeq ($(TARGET_CPU),x86_64)
> +DARWIN_EXE_LDFLAGS += -pagezero_size 10000 -image_base 100000000

Can you comment why this is needed?

@@ +38,5 @@
> +
> +GARBAGE += $(DEPTH)/$(PROGRAM) gdb-tests-gdb.py.tmp
> +
> +gdb-tests-gdb.py: gdb-tests-gdb.py.in
> +	sed -e 's|@jsgdbdir@|$(abspath $(topsrcdir))/gdb|' < $< > $@.tmp && mv $@.tmp $@

I think there might be some build system magic gunk for doing part of this (esp the move-if-diff), but I'll let a build peer comment.

Also, I'd personally prefer this to be done through the configure-based .in rewriting. Do an AC_SUBST(jsgdbdir) or whatever.

::: js/src/gdb/TODO
@@ +1,2 @@
> +* Before landing:
> +- jschar *

I assume this is obsolete one way or another, since you're coming in for a landing now?

::: js/src/gdb/gdb-tests.h
@@ +27,5 @@
> +        allFragments = this;
> +    }
> +
> +    // The name of this fragment. gdb-tests.cpp runs the fragments whose names are
> +    // passed it on the command line.

s/it/in/, or "gdb-tests.cpp runs the fragment names on the command line." or somesuch.

::: js/src/gdb/lib-for-tests/prolog.py
@@ +4,5 @@
> +import traceback
> +
> +# Run the C++ fragment named |fragment|, stopping on entry to |function|,
> +# and then select the calling frame.
> +def run_fragment_to_hit(fragment, function='breakpoint'):

I initially parsed this function name wrong. "Run the fragment that you want to hit"?

run_fragment_to_breakpoint? run_fragment_until_hit? run_fragment_until_breakpoint_hit_and_everyone_agrees_that_the_longer_the_identifier_the_better?

I'd vote for the 1st.

@@ +34,5 @@
> +# |printer|, with a subprinter named |subprinter|.
> +def assert_subprinter_registered(printer, subprinter):
> +    # Match a line containing |printer| followed by a colon, and then a
> +    # series of more-indented lines containing |subprinter|.
> +    pat = r'^( +)%s *\n(\1 +.*\n)*\1 +%s *\n' % (re.escape(printer), re.escape(subprinter))

This is grotesque. r=me.

Though I think I'd weakly prefer to embed the match strings:

  names = { 'printer': re.escape(printer), 'subprinter': re.escape(subprinter) }
  pat = r'^( +)%(printer)s *\n(\1 +.*\n)*\1 +%(subprinter)s *\n' % names

::: js/src/gdb/mozilla/Root.py
@@ +26,5 @@
> +            ptr = ptr.dereference()
> +        # As of 2012-11, GDB suppresses printing pointers in replacement values;
> +        # see http://sourceware.org/ml/gdb/2012-11/msg00055.html That means that
> +        # simply returning the 'ptr' member won't work. Instead, just invoke
> +        # GDB's formatter ourselves.

Ohh.... that explains something that's been bugging me for a while.

@@ +27,5 @@
> +        # As of 2012-11, GDB suppresses printing pointers in replacement values;
> +        # see http://sourceware.org/ml/gdb/2012-11/msg00055.html That means that
> +        # simply returning the 'ptr' member won't work. Instead, just invoke
> +        # GDB's formatter ourselves.
> +        return str(ptr)

My version of these things added in a descriptive string for the container type. But with this new code, it's clear where to add that if I feel the need.

::: js/src/gdb/mozilla/autoload.py
@@ +16,5 @@
> +    import my_mozilla_printers
> +except ImportError:
> +    pass
> +
> +# Register our pretty-printers with |objfile|.

I know this is weird, but given that importing the mozilla.* modules has a side-effect of registering themselves, could you move the definition of |register| up before the mozilla.* imports? (But after the mozilla.prettyprinters import.)

::: js/src/gdb/mozilla/prettyprinters.py
@@ +13,5 @@
> +# other name already. (We attach 'enabled' flags to the function objects
> +# themselves, and a single function can't carry the 'enabled' flags for two
> +# different printers.)
> +def check_for_reused_pretty_printer(fn):
> +    if hasattr(fn, 'enabled'):

Is the 'enabled' attribute redundant with whether it exists in the subprinters list?

@@ +81,5 @@
> +# module like this:
> +#
> +#   clear_module_printers(__name__)
> +def clear_module_printers(module_name):
> +    global printers_by_tag, ptr_printers_by_tag, template_printers_by_tag, printers_by_regexp

I hate Python's scoping rules.

@@ +117,5 @@
> +subprinters = []
> +
> +# Set up the 'name' and 'enabled' attributes on |subprinter|, and add it to our
> +# list of all SpiderMonkey subprinters.
> +def list_subprinter(subprinter, name):

Using 'list' as a verb is confusing, since it doesn't mean what I'd expect (to passively enumerate the "elements" of a subprinter or something). 'enlist', at least, or perhaps a different verb?

@@ +153,5 @@
> +        # the objfile in whose scope lookups should occur. But simply
> +        # knowing that we need to lookup the types afresh is probably
> +        # enough.
> +        self.void_t = gdb.lookup_type('void')
> +        self.void_ptr_t = self.void_t.pointer()

yay! I felt queasy every time I did a gdb.lookup_type() in the middle of a to_string.

@@ +197,5 @@
> +
> +    yield t
> +    for t2 in followers(t): yield t2
> +
> +template_regexp = re.compile("([a-zA-Z0-9_:]+)<")

a-z makes me nervous in a Unicode world, even though it doesn't matter here. Can you make this r'([\w_:]+)<' instead? (The underscore appears to be redundant with \w, which surprises me. But I'd rather have it explicitly present.)

@@ +204,5 @@
> +def lookup_for_objfile(objfile):
> +    # Create a type cache for this objfile.
> +    try:
> +        cache = TypeCache(objfile)
> +    except NotSpiderMonkeyObjfileError:

I don't see where that exception is ever thrown.

@@ +251,5 @@
> +        s = str(value.type)
> +        for (r, f) in printers_by_regexp:
> +            if f.enabled:
> +                m = r.match(s)
> +                if m and m.end() == s.len:

You don't want to put the onus on the regexp author to anchor the end? This prevents writing some types of matches, but perhaps it's better to prevent that whole class of errors.

@@ +282,5 @@
> +#     description of the referent.
> +#
> +#     Note that pretty-printers returning a 'string' display hint must not use
> +#     this default 'to_string' method, as GDB will take everything it returns,
> +#     including the type name and address, as string contents.

I almost want an assertion for this, but it seems like more trouble than it's worth.

::: js/src/gdb/mozilla/struct.py
@@ +1,1 @@
> +# mozilla/struct.py - utilities for printing SpiderMonkey structure types.

Do you have further patches that put something here? I don't think anything in this patch imports it.

::: js/src/gdb/progressbar.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Text progress bar library, like curl or scp.

We really ought to common up this and js/src/tests/lib/progressbar.py, though this one's a lot simpler.

::: js/src/gdb/run-tests.py
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# run-tests.py -- Python harness for GDB SpiderMonkey support

I'll confess I barely skimmed this file.

@@ +155,5 @@
> +        testlibdir = os.path.normpath(os.path.join(OPTIONS.testdir, '..', 'lib-for-tests'))
> +        return [OPTIONS.gdb_executable,
> +                '-nw',          # Don't create a window (unnecessary?)
> +                '-nx',          # Don't read .gdbinit.
> +                '--ex', 'add-auto-load-safe-path %s' % (OPTIONS.builddir,),

I would make fun of you for the extra listification, but I do the same thing.

@@ +245,5 @@
> +    op.add_option('-x', '--exclude', dest='exclude', action='append',
> +                  help='exclude given test dir or path')
> +    op.add_option('-t', '--timeout', dest='timeout',  type=float, default=150.0,
> +                  help='set test timeout in seconds')
> +    op.add_option('-j', '--worker-count', dest='workercount', type=int, default=4,

It probably doesn't add much, but you might want to look at js/src/tests/jstests.py's get_cpu_count() for an alternative default.

::: js/src/gdb/taskpool.py
@@ +1,5 @@
> +import fcntl, os, select, time
> +from subprocess import Popen, PIPE
> +
> +# Run a series of subprocesses. Try to keep up to a certain number going in
> +# parallel at any given time. Enforce time limits.

You implemented this from scratch, didn't you? I think it overlaps some with a couple of other attempts. js/src/tests/lib/tasks_unix.py, for example, and gps or somebody was at least talking about another implementation.

Oh well. Works for me. I didn't scrutinize this closely, but it seems vaguely sane.

@@ +5,5 @@
> +# parallel at any given time. Enforce time limits.
> +#
> +# This is implemented using non-blocking I/O, and so is Unix-specific.
> +#
> +# We assume that, if a task closes its standard output, then it's safe to

s/standard output/standard error/?

@@ +109,5 @@
> +                (readable,w,x) = select.select(stdouts_and_stderrs, [], [], secs_to_next_deadline)
> +                finished = set()
> +                terminate = set()
> +                for t in running:
> +                    # Since we've place the pipes in non-blocking mode, thes

s/place/placed/
s/thes/these/

@@ +160,5 @@
> +                running -= finished
> +        return None
> +
> +if __name__ == '__main__':
> +    # Test TaskPool by using it to implement the unique 'sleep sort' algorithm.

:-)

::: js/src/gdb/tests/test-Root-null.py
@@ +1,5 @@
> +# Test printing roots that refer to NULL pointers.
> +
> +# Disable the JSObject * pretty-printer. This isn't about whether the
> +# Rooted's referent's pretty-printer is null-safe.
> +gdb.execute('disable pretty-printer .*/gdb-tests SpiderMonkey;ptr-to-JSObject')

Huh, what?

::: js/src/shell/Makefile.in
@@ +60,5 @@
>  	$(SYSINSTALL) $^ $(DESTDIR)$(bindir)
> +
> +
> +js-gdb.py: js-gdb.py.in
> +	sed -e 's|@jsgdbdir@|$(abspath $(topsrcdir))/gdb|' < $< > $@.tmp && mv $@.tmp $@

Again, seems like this should be done via configure.

Comment 4

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #3)
> This much looks beautiful. r=me for the non-build parts. Though this is
> still missing quite a bit of functionality from your original version.

Yes --- I left out a bunch of stuff that I didn't understand, or felt was under-tested, or was badly designed. (The struct/union printing stuff, in particular, was super-hairy and didn't feel well-motivated at all.)

> Do you think it would be ok to build all of the gdb tests as part of either
> the default tinderbox build or make check? I'd sort of like people to know
> immediately if the test programs don't even compile anymore. I don't think
> people are ready to backout if the gdb tests fail, but breaking the
> compilation shouldn't be too bad.

gdb-tests and its object files are indeed built as part of the ordinary 'make' process. If you do a build with this patch applied, you'll get a 'gdb' subdirectory of your object directory, with .o files and an executable.

> ::: js/src/Makefile.in
> @@ +31,4 @@
> >  TEST_DIRS += jsapi-tests
> >  endif
> >  
> > +TEST_DIRS += tests gdb
> 
> This is going to require a build peer review.

The patch hunk, as quoted above, is not accurate; it just adds gdb/Makefile.in to the mix. But, yes, it's a whole new Makefile, so it'd be good to get a reviewer who feels comfortable with that.

> ::: js/src/gdb/Makefile.in
> @@ +27,5 @@
> > +LOCAL_INCLUDES += -I$(topsrcdir) -I..
> > +
> > +ifeq ($(OS_ARCH),Darwin)
> > +ifeq ($(TARGET_CPU),x86_64)
> > +DARWIN_EXE_LDFLAGS += -pagezero_size 10000 -image_base 100000000
> 
> Can you comment why this is needed?

... I just cargo-culted it from js/Makefile.in. Actually, I don't see why it would be needed, since it's not in jsapi-tests/Makefile.in. I'll take it out and see what happens.

> @@ +38,5 @@
> > +
> > +GARBAGE += $(DEPTH)/$(PROGRAM) gdb-tests-gdb.py.tmp
> > +
> > +gdb-tests-gdb.py: gdb-tests-gdb.py.in
> > +	sed -e 's|@jsgdbdir@|$(abspath $(topsrcdir))/gdb|' < $< > $@.tmp && mv $@.tmp $@
> 
> I think there might be some build system magic gunk for doing part of this
> (esp the move-if-diff), but I'll let a build peer comment.

Yes; I'll find out about that.

> Also, I'd personally prefer this to be done through the configure-based .in
> rewriting. Do an AC_SUBST(jsgdbdir) or whatever.

That's a possibility I hadn't considered.

> ::: js/src/gdb/TODO
> @@ +1,2 @@
> > +* Before landing:
> > +- jschar *
> 
> I assume this is obsolete one way or another, since you're coming in for a
> landing now?

I moved this to the non-pre-land list.

> ::: js/src/gdb/gdb-tests.h
> @@ +27,5 @@
> > +        allFragments = this;
> > +    }
> > +
> > +    // The name of this fragment. gdb-tests.cpp runs the fragments whose names are
> > +    // passed it on the command line.
> 
> s/it/in/, or "gdb-tests.cpp runs the fragment names on the command line." or
> somesuch.

I did s/it/to it/.

> ::: js/src/gdb/lib-for-tests/prolog.py
> @@ +4,5 @@
> > +import traceback
> > +
> > +# Run the C++ fragment named |fragment|, stopping on entry to |function|,
> > +# and then select the calling frame.
> > +def run_fragment_to_hit(fragment, function='breakpoint'):
> 
> I initially parsed this function name wrong. "Run the fragment that you want
> to hit"?
> 
> run_fragment_to_breakpoint? run_fragment_until_hit?
> run_fragment_until_breakpoint_hit_and_everyone_agrees_that_the_longer_the_ide
> ntifier_the_better?
> 
> I'd vote for the 1st.

How about just 'run_fragment'?

> @@ +34,5 @@
> > +# |printer|, with a subprinter named |subprinter|.
> > +def assert_subprinter_registered(printer, subprinter):
> > +    # Match a line containing |printer| followed by a colon, and then a
> > +    # series of more-indented lines containing |subprinter|.
> > +    pat = r'^( +)%s *\n(\1 +.*\n)*\1 +%s *\n' % (re.escape(printer), re.escape(subprinter))
> 
> This is grotesque. r=me.
> 
> Though I think I'd weakly prefer to embed the match strings:
> 
>   names = { 'printer': re.escape(printer), 'subprinter':
> re.escape(subprinter) }
>   pat = r'^( +)%(printer)s *\n(\1 +.*\n)*\1 +%(subprinter)s *\n' % names

I like that better, too. Changed. (My Python-fu is not strong.)

> ::: js/src/gdb/mozilla/Root.py
> @@ +26,5 @@
> > +            ptr = ptr.dereference()
> > +        # As of 2012-11, GDB suppresses printing pointers in replacement values;
> > +        # see http://sourceware.org/ml/gdb/2012-11/msg00055.html That means that
> > +        # simply returning the 'ptr' member won't work. Instead, just invoke
> > +        # GDB's formatter ourselves.
> 
> Ohh.... that explains something that's been bugging me for a while.

I was irritated when I found the change in GDB, and I felt the review leading to it was pretty weak. "Sure, whatever."

> @@ +27,5 @@
> > +        # As of 2012-11, GDB suppresses printing pointers in replacement values;
> > +        # see http://sourceware.org/ml/gdb/2012-11/msg00055.html That means that
> > +        # simply returning the 'ptr' member won't work. Instead, just invoke
> > +        # GDB's formatter ourselves.
> > +        return str(ptr)
> 
> My version of these things added in a descriptive string for the container
> type. But with this new code, it's clear where to add that if I feel the
> need.

I considered that. Patch welcome!

> ::: js/src/gdb/mozilla/autoload.py
> @@ +16,5 @@
> > +    import my_mozilla_printers
> > +except ImportError:
> > +    pass
> > +
> > +# Register our pretty-printers with |objfile|.
> 
> I know this is weird, but given that importing the mozilla.* modules has a
> side-effect of registering themselves, could you move the definition of
> |register| up before the mozilla.* imports? (But after the
> mozilla.prettyprinters import.)

I don't mind, but what's the relationship? That 'register' is only called by the FOO-js.py files. It's not what the pretty-printers call to register themselves. Perhaps something needs to be renamed?

> ::: js/src/gdb/mozilla/prettyprinters.py
> @@ +13,5 @@
> > +# other name already. (We attach 'enabled' flags to the function objects
> > +# themselves, and a single function can't carry the 'enabled' flags for two
> > +# different printers.)
> > +def check_for_reused_pretty_printer(fn):
> > +    if hasattr(fn, 'enabled'):
> 
> Is the 'enabled' attribute redundant with whether it exists in the
> subprinters list?

No: its presence in the subprinters list determines whether 'info pretty-printer' lists it, and whether 'enable pretty-printer' and 'disable pretty-printer' can control it; its 'enabled' attribute determines is what those commands show and change.

> @@ +81,5 @@
> > +# module like this:
> > +#
> > +#   clear_module_printers(__name__)
> > +def clear_module_printers(module_name):
> > +    global printers_by_tag, ptr_printers_by_tag, template_printers_by_tag, printers_by_regexp
> 
> I hate Python's scoping rules.

Yep.

> @@ +117,5 @@
> > +subprinters = []
> > +
> > +# Set up the 'name' and 'enabled' attributes on |subprinter|, and add it to our
> > +# list of all SpiderMonkey subprinters.
> > +def list_subprinter(subprinter, name):
> 
> Using 'list' as a verb is confusing, since it doesn't mean what I'd expect
> (to passively enumerate the "elements" of a subprinter or something).
> 'enlist', at least, or perhaps a different verb?

Excellent point. I've changed these to 'add_to_subprinter_list' and 'remove_from_subprinter_list'.

> @@ +153,5 @@
> > +        # the objfile in whose scope lookups should occur. But simply
> > +        # knowing that we need to lookup the types afresh is probably
> > +        # enough.
> > +        self.void_t = gdb.lookup_type('void')
> > +        self.void_ptr_t = self.void_t.pointer()
> 
> yay! I felt queasy every time I did a gdb.lookup_type() in the middle of a
> to_string.

I did, too. :)

> @@ +197,5 @@
> > +
> > +    yield t
> > +    for t2 in followers(t): yield t2
> > +
> > +template_regexp = re.compile("([a-zA-Z0-9_:]+)<")
> 
> a-z makes me nervous in a Unicode world, even though it doesn't matter here.
> Can you make this r'([\w_:]+)<' instead? (The underscore appears to be
> redundant with \w, which surprises me. But I'd rather have it explicitly
> present.)

Done.

> @@ +204,5 @@
> > +def lookup_for_objfile(objfile):
> > +    # Create a type cache for this objfile.
> > +    try:
> > +        cache = TypeCache(objfile)
> > +    except NotSpiderMonkeyObjfileError:
> 
> I don't see where that exception is ever thrown.

D'oh! Added.

Will address the remaining comments later; posting what I've got now.

Comment 5

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #3)
> @@ +251,5 @@
> > +        s = str(value.type)
> > +        for (r, f) in printers_by_regexp:
> > +            if f.enabled:
> > +                m = r.match(s)
> > +                if m and m.end() == s.len:
> 
> You don't want to put the onus on the regexp author to anchor the end? This
> prevents writing some types of matches, but perhaps it's better to prevent
> that whole class of errors.

Well, since it's not what 'match' does by default, people probably aren't expecting that extra constraint. I'll drop it.

> @@ +282,5 @@
> > +#     description of the referent.
> > +#
> > +#     Note that pretty-printers returning a 'string' display hint must not use
> > +#     this default 'to_string' method, as GDB will take everything it returns,
> > +#     including the type name and address, as string contents.
> 
> I almost want an assertion for this, but it seems like more trouble than
> it's worth.

I added an assertion.

> ::: js/src/gdb/mozilla/struct.py
> @@ +1,1 @@
> > +# mozilla/struct.py - utilities for printing SpiderMonkey structure types.
> 
> Do you have further patches that put something here? I don't think anything
> in this patch imports it.

Oh --- I was getting ready to do some of the struct interior pretty-printers. I never got around to it. Sorry I missed this. I've deleted the file.

> ::: js/src/gdb/progressbar.py
> @@ +1,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +# Text progress bar library, like curl or scp.
> 
> We really ought to common up this and js/src/tests/lib/progressbar.py,
> though this one's a lot simpler.

This is a copy of the one in js/src/jit-test, with a small extension. Perhaps we could put all those things in a common js/src/python directory?

> ::: js/src/gdb/run-tests.py
> @@ +2,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +# run-tests.py -- Python harness for GDB SpiderMonkey support
> 
> I'll confess I barely skimmed this file.

I don't blame you.

> @@ +155,5 @@
> > +        testlibdir = os.path.normpath(os.path.join(OPTIONS.testdir, '..', 'lib-for-tests'))
> > +        return [OPTIONS.gdb_executable,
> > +                '-nw',          # Don't create a window (unnecessary?)
> > +                '-nx',          # Don't read .gdbinit.
> > +                '--ex', 'add-auto-load-safe-path %s' % (OPTIONS.builddir,),
> 
> I would make fun of you for the extra listification, but I do the same thing.

Yeah... I don't want the behavior to depend on the type OPTIONS.builddir happens to have.

> @@ +245,5 @@
> > +    op.add_option('-x', '--exclude', dest='exclude', action='append',
> > +                  help='exclude given test dir or path')
> > +    op.add_option('-t', '--timeout', dest='timeout',  type=float, default=150.0,
> > +                  help='set test timeout in seconds')
> > +    op.add_option('-j', '--worker-count', dest='workercount', type=int, default=4,
> 
> It probably doesn't add much, but you might want to look at
> js/src/tests/jstests.py's get_cpu_count() for an alternative default.

Okay, I've copied that. I'll file a followup bug for the duplication in our Python.

> ::: js/src/gdb/taskpool.py
> @@ +1,5 @@
> > +import fcntl, os, select, time
> > +from subprocess import Popen, PIPE
> > +
> > +# Run a series of subprocesses. Try to keep up to a certain number going in
> > +# parallel at any given time. Enforce time limits.
> 
> You implemented this from scratch, didn't you? I think it overlaps some with
> a couple of other attempts. js/src/tests/lib/tasks_unix.py, for example, and
> gps or somebody was at least talking about another implementation.
> 
> Oh well. Works for me. I didn't scrutinize this closely, but it seems
> vaguely sane.

Yeah. Mine does use the new subprocess.Popen module, which is the new spiffy way to do things. And I think it's a nicer abstraction than tasks_unix.py.

> @@ +5,5 @@
> > +# parallel at any given time. Enforce time limits.
> > +#
> > +# This is implemented using non-blocking I/O, and so is Unix-specific.
> > +#
> > +# We assume that, if a task closes its standard output, then it's safe to
> 
> s/standard output/standard error/?

Yes --- thanks!

> s/place/placed/
> s/thes/these/

Fixed --- thanks.

> @@ +160,5 @@
> > +                running -= finished
> > +        return None
> > +
> > +if __name__ == '__main__':
> > +    # Test TaskPool by using it to implement the unique 'sleep sort' algorithm.
> 
> :-)

I don't see why I should re-implement sort if the kernel has already implemented it. (Idea from a Rust demo.)

> ::: js/src/gdb/tests/test-Root-null.py
> @@ +1,5 @@
> > +# Test printing roots that refer to NULL pointers.
> > +
> > +# Disable the JSObject * pretty-printer. This isn't about whether the
> > +# Rooted's referent's pretty-printer is null-safe.
> > +gdb.execute('disable pretty-printer .*/gdb-tests SpiderMonkey;ptr-to-JSObject')
> 
> Huh, what?

I replaced the comment with this:

# Since mozilla.prettyprinters.Pointer declines to create pretty-printers
# for null pointers, GDB built-in printing code ends up handling them. But
# as of 2012-11, GDB suppresses printing pointers in replacement values:
# see: http://sourceware.org/ml/gdb/2012-11/msg00055.html
#
# Thus, if the pretty-printer for js::Rooted simply returns the referent as
# a replacement value (which seems reasonable enough, if you want the
# pretty-printer to be completely transparent), and the referent is a null
# pointer, it prints as nothing at all.
#
# This test ensures that the js::Rooted pretty-printer doesn't make that
# mistake.

> ::: js/src/shell/Makefile.in
> @@ +60,5 @@
> >  	$(SYSINSTALL) $^ $(DESTDIR)$(bindir)
> > +
> > +
> > +js-gdb.py: js-gdb.py.in
> > +	sed -e 's|@jsgdbdir@|$(abspath $(topsrcdir))/gdb|' < $< > $@.tmp && mv $@.tmp $@
> 
> Again, seems like this should be done via configure.

I talked with ted and found the right approach. I'll use this:

http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1562

Comment 6

[Jim Blandy :jimb]

In order to use the PP_TARGETS approach, I need the automatic '.in' suffix stripping implemented by the patch in bug 701393.

Comment 7

[Jim Blandy :jimb]

Attachment: GDB pretty-printing support for SpiderMonkey.

Revised per sfink's comments.

Ted, all we need review on here from you is the Makefile bits.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 687930 [details] [diff] [review]
GDB pretty-printing support for SpiderMonkey.

Review of attachment 687930 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gdb/Makefile.in
@@ +19,5 @@
> +                  test-jsval.cpp                \
> +                  test-prettyprinters.cpp       \
> +                  test-Root.cpp                 \
> +                  typedef-printers.cpp          \
> +                  $(NULL)

Current Makefile style is:
CPPSRCS = \
  gdb-tests.cpp \
  ...
  $(NULL)

2-space indent, no assignment on the first line if you're using continuations.

@@ +24,5 @@
> +
> +DEFINES         += -DEXPORT_JS_API
> +# Building against js_static requires that we declare mfbt sybols "exported"
> +# on its behalf.
> +DEFINES         += -DIMPL_MFBT

Might as well just combine these into one assignment.

Comment 9

[Jim Blandy :jimb]

Thanks for the reviews, Steve and Ted! I've made the changes you suggest.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8722babb63

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d8722babb63