899342 - Let 'mach debug' pass arguments to GDB

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jim Blandy :jimb]

Attachment: mach-debug-gdbparams.patch

'mach debug' is handy, but it's even handier if one can use it within Emacs. All that's required is the ability to pass the '-fullname' flag to GDB, before the actual command and arguments.

This patch adds a '+gdbparams' argument to 'mach debug' whose value is split at whitespace and passed to GDB before the '--args' argument. 

Note that if we support other debuggers, like lldb, they can ignore the +gdbparams argument, and take their own flag, so it'll be easy to write mach-based scripts that get the right behavior out of one debugger without confusing the others.

Comment 1

[Gregory Szorc [:gps]]

mach bugs go in the component that the command is related to (unless it is a bug with the mach core - which is rare).

Comment 2

[Jim Blandy :jimb]

An alternative approach would be for mach to take a +emacs argument, and implement it for all debuggers (or report an error if it can't do so).

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 782804 [details] [diff] [review]
mach-debug-gdbparams.patch

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

LGTM. I assume you wanted review.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +650,5 @@
>      @CommandArgument('+background', '+b', action='store_true',
>          help='Do not pass the -foreground argument by default on Mac')
> +    @CommandArgument('+gdbparams', default=None, metavar='params', type=str,
> +        help='Command-line arguments to pass to GDB itself; split at whitespace.')
> +    def debug(self, params, remote, background, gdbparams):

We should probably be using named arguments instead of positional. I'm kinda surprised this works!

@@ +660,5 @@
>              print(e)
>              return 1
> +        args = [debugger]
> +        if gdbparams:
> +            args.extend(gdbparams.split())

How do you feel about using shlex.split() instead? docs.python.org/2/library/shlex.html

Comment 4

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #3)
> How do you feel about using shlex.split() instead?
> docs.python.org/2/library/shlex.html

shlex is useless. it doesn't handle quotes. pymake.process.clinetoargv does, but it doesn't like some characters.

Comment 5

[Jim Blandy :jimb]

(In reply to Gregory Szorc [:gps] from comment #3)
> Comment on attachment 782804 [details] [diff] [review]
> mach-debug-gdbparams.patch
> 
> Review of attachment 782804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. I assume you wanted review.

Yes - was going to find out who to ask.

> ::: python/mozbuild/mozbuild/mach_commands.py
> @@ +650,5 @@
> >      @CommandArgument('+background', '+b', action='store_true',
> >          help='Do not pass the -foreground argument by default on Mac')
> > +    @CommandArgument('+gdbparams', default=None, metavar='params', type=str,
> > +        help='Command-line arguments to pass to GDB itself; split at whitespace.')
> > +    def debug(self, params, remote, background, gdbparams):
> 
> We should probably be using named arguments instead of positional. I'm kinda
> surprised this works!

I'm cargo-culting; if you can be a bit more specific about what should be changed, I'll happily fix it.

Comment 6

[Jim Blandy :jimb]

Attachment: Implement '+gdbparams' argument for 'mach debug'.

Revised to use pymake.process.clinetoargv.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 783913 [details] [diff] [review]
Implement '+gdbparams' argument for 'mach debug'.

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

r+ with moved import.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +8,4 @@
>  import operator
>  import os
>  import sys
> +import pymake.process

All non-stdlib and non-mach and non-mozbuild imports in mach_commands.py files should go inside the command function. This prevents import bloat at mach execution time.

Comment 8

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/780c67f5b185

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/780c67f5b185