automationutils.py has facilities for running e.g. firefox in a debugger: http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#59 http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#263 This should be ported to mozrunner. If a debugger is chosen, the application should not time out and it should be appropriately interactive depending on the debugger chosen

See also: http://code.google.com/p/pydevtools/source/browse/trunk/bintools/gdb/mi.py?r=21

Currently we hang on the select statement https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L704 We don't get the (gdb) prompt

Something is definitely "wonky" with the way mozprocess deals with stdin/stdout, especially for the interactive case. However, we don't really need it for the interactive debugger case. I think for the immediate term I'm going to just use subprocess.Popen for this since we don't really need process management here

We talked about this in person, and I'm in full agreement. We don't need any of the niceties of mozprocess in the interactive debugger case, so it's kind of silly to try to hack this functionality in there.

I'm not fully convinced that this is a great implementation, but I figured i'd put it up and hopefully get some feedback. I've tested this with gdb and it works. My valgrind installation doesn't work with or without mozrunner (I'll attach the output subsequently).

│valgrind --leak-check=full `which firefox` ==27983== Memcheck, a memory error detector ==27983== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al. ==27983== Using Valgrind-3.6.1-Debian and LibVEX; rerun with -h for copyright info ==27983== Command: /home/jhammel/firefox/firefox ==27983== valgrind: Fatal error at startup: a function redirection valgrind: which is mandatory for this platform-tool combination valgrind: cannot be set up. Details of the redirection are: valgrind: valgrind: A must-be-redirected function valgrind: whose name matches the pattern: index valgrind: in an object with soname matching: ld-linux.so.2 valgrind: was not found whilst processing valgrind: symbols from the object with soname: ld-linux.so.2 valgrind: valgrind: Possible fixes: (1, short term): install glibc's debuginfo valgrind: package on this machine. (2, longer term): ask the packagers valgrind: for your Linux distribution to please in future ship a non- valgrind: stripped ld.so (or whatever the dynamic linker .so is called) valgrind: that exports the above-named function using the standard valgrind: calling conventions for this platform. The package you need valgrind: to install for fix (1) is called valgrind: valgrind: On Debian, Ubuntu: libc6-dbg valgrind: On SuSE, openSuSE, Fedora, RHEL: glibc-debuginfo valgrind: valgrind: Cannot continue -- exiting now. Sorry. FWIW, I do have libc6-dbg installed (ubuntu 11.10)

In all likeliness, your system is 64-bits, and your firefox 32-bits. Which means you need the 32-bits debugging symbols for the libc.

Comment on attachment 616619 [details] [diff] [review] implementation Review of attachment 616619 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozrunner/mozrunner/runner.py @@ +67,5 @@ > +def debugger_arguments(debugger): > + """default debugger arugments""" > + > + if debugger not in debuggers: > + raise AssertionError("Debugger '%s' not found (Available: %s)" % (debugger, ', '.join(debuggers.keys()))) For automation[utils], we don't treat this as an exhaustive list, it's simply to be able to say --debugger=gdb instead of having to supply the full list of required args and the interactive flag. I think you should allow the same behavior of specifying arbitrary debugger executables. @@ +192,5 @@ > return > + if isinstance(self.process_handler, subprocess.Popen): > + self.process_handler.wait() > + else: > + self.process_handler.waitForFinish(timeout=timeout, outputTimeout=outputTimeout) This is a bit less-than-ideal, but without subclassing Popen for the interactive case there's probably not a great way to make this cleaner. @@ +312,5 @@ > default=[], action='append', > help="provides an argument to the test application") > + parser.add_option('--debugger', dest='debugger', type='choice', > + choices=debuggers.keys(), > + help="run under a debugger") You'll note that automationutils also provides --debugger-args and --debugger-interactive options. Those would make it easier to use arbitrary debugger programs.

Good point about supporting --debugger-args and --debugger-interactive. How should this work (for now) in mozrunner? --debugger-interactive (or --debugger-noninteractive for the reverse case) could be used to override that of --debugger. OTOH, --debugger-args and --debugger are a bit confusing. We could err out on combining them, or (guessing you'll think this is too implicit) if --debugger and --debugger-args are specified the latter can be overrides to the particular debugger. Also, automationutils.py accepts debuggerArgs as a string which it then splits on spaces (wrong if you have any arguments that have spaces in them). Currently we don't do this in any other place in mozbase and it would be nice to solidify on a arg-passing strategy. Should we use, e.g. --debugger-args `which gdb` --debugger-args --someflag --debugger-args -x --debugger-args 'A file\with spaces.gdb' to get, e.g. ['/usr/bin/gdb', '--someflag', '-x', 'A file with\spaces.gdb']

I wouldn't worry *too* much about overriding the default "interactive", say. That's mostly useful for specifying some arbitrary command as a debugger. The --debugger/--debugger-args stuff is a useful split. Say if you want to run valgrind with the stock args, you just specify --debugger=valgrind. If you want to use some different arguments, you can also pass --debugger-args. I guess you're saying we could just combine them and say --debugger=valgrind --debugger=--something --debugger=--something-else? That seems a little weird. The string vs. list thing is kind of fiddly. It doesn't seem like it's a problem in practice, and making it a list just forces people to write extra arguments to get what they want. (I know, I know, ideological purity and all...)

WRT verbosity, :aki has a class in mozharness that uses csv vs multiple (e.g.) --debugger-args lines: http://hg.mozilla.org/build/mozharness/file/203c495e262b/mozharness/base/config.py#l48 I'd rather take this across the board for mozbase, if we decide we want to go this direction, instead of trying to cram into the scope of this bug. There is the niche case where you have a comma that is part of your value, though I can't think of any case OTTOMH where we care about this. When we get python 2.7, argparse allows more flexibility here so we could just wait until that. Nits aside, I'll implement these and we can iterate on a solution here.

This allows --debugger with whatever --debugger-args you want, with the caveat that you need at least one (!). Not sure if this is exactly what we want to do, but its pretty flexible

Comment on attachment 621792 [details] [diff] [review] include --debugger-args and interactive Review of attachment 621792 [details] [diff] [review]: ----------------------------------------------------------------- I have some nitpicky comments, but this looks fine. ::: mozrunner/mozrunner/runner.py @@ +73,5 @@ > + """ > + > + # find debugger executable if not a file > + executable = debugger > + if not os.path.exists(executable): Sort of a weird edgecase, but if you specified a relative path here, should we make it absolute? @@ +81,5 @@ > + > + # if debugger not in dictionary of knowns return defaults > + dirname, debugger = os.path.split(debugger) > + if debugger not in debuggers: > + return ([executable] + (arguments or []), bool(interactive)) Might want to default to interactive, since that's probably more useful for debuggers, but it's kind of arbitrary. @@ +196,5 @@ > + > + # > + if interactive: > + self.process_handler = subprocess.Popen(cmd, env=self.env) > + # TODO: other arguments This could probably use a comment explaining why we're skipping mozprocess. @@ +388,5 @@ > + # attach a debugger if specified > + debug_args = self.options.debugger_args > + interactive = self.options.interactive > + if self.options.debugger: > + debug_args, interactive = debugger_arguments(self.options.debugger) Do you want to pass debug_args/interactive to debugger_arguments so that commandline options override the defaults?

(In reply to Ted Mielczarek [:ted] from comment #13) > Comment on attachment 621792 [details] [diff] [review] > include --debugger-args and interactive > > Review of attachment 621792 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have some nitpicky comments, but this looks fine. > > ::: mozrunner/mozrunner/runner.py > @@ +73,5 @@ > > + """ > > + > > + # find debugger executable if not a file > > + executable = debugger > > + if not os.path.exists(executable): > > Sort of a weird edgecase, but if you specified a relative path here, should > we make it absolute? Is there a reason to do so? > @@ +81,5 @@ > > + > > + # if debugger not in dictionary of knowns return defaults > > + dirname, debugger = os.path.split(debugger) > > + if debugger not in debuggers: > > + return ([executable] + (arguments or []), bool(interactive)) > > Might want to default to interactive, since that's probably more useful for > debuggers, but it's kind of arbitrary. I'm going with what is currently in http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#165 except with a shorter option name. > @@ +196,5 @@ > > + > > + # > > + if interactive: > > + self.process_handler = subprocess.Popen(cmd, env=self.env) > > + # TODO: other arguments > > This could probably use a comment explaining why we're skipping mozprocess. Will add. > @@ +388,5 @@ > > + # attach a debugger if specified > > + debug_args = self.options.debugger_args > > + interactive = self.options.interactive > > + if self.options.debugger: > > + debug_args, interactive = debugger_arguments(self.options.debugger) > > Do you want to pass debug_args/interactive to debugger_arguments so that > commandline options override the defaults? Yes, I believe so. Otherwise there is no way to override from the command line.

pushed: https://github.com/mozilla/mozbase/commit/913a4e5853c67169d71fc219f083d8661b54c3d8 If we want any follow ups we can take them

closing