Makes android.py support gdb

Makes android.py support gdb

The gdb dance is a bit cryptic. I have to do the following:
. remap local port to remote port (this really only needs to be done once)
. wait to figure out process id once started on device.
. Start gdbserver on device.
. Start gdb on local device.

Additionally the names of the shared libraries don't align, eg we end
up with html_viewer.mojo on disk but the library that comes from it is
html_viewer_library.so. For this reason I create a temp directory and
point the local gdb at it, creating links to the real files.

R=msw@chromium.org
BUG=none
TEST=none

Committed: https://crrev.com/2c7d24cd78f53c232d0c80585e8129f854af1f43
Cr-Commit-Position: refs/heads/master@{#331146}

[msw, 2015-05-22 02:41:44]

Maybe document the gdb flow in a code comment?

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
File mandoline/tools/android_run_mandoline.py (right):

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
mandoline/tools/android_run_mandoline.py:33:
parser.add_argument('--dont-install',
q: just curious, is this needed or just for efficiency?

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
mandoline/tools/android_run_mandoline.py:59: shell.StartShell(args, sys.stdout,
p.terminate, gdb=launcher_args.gdb)
nit: you don't actually need "gdb=" here

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/android_mojo_she...
File mojo/tools/android_mojo_shell.py (right):

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/android_mojo_she...
mojo/tools/android_mojo_shell.py:39: launcher_args.target_device,
src_root=paths.src_root)
nit: not needed, or maybe add the remaining gdb support bits here?

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py
File mojo/tools/mopy/android.py (right):

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:184: class Tmpdir(object):
nit: Maybe just inline this in _WaitForProcessIdAndStartGdb:
  temp_dir = tempfile.mkdtemp()
  atexit.register(shutil.rmtree, temp_dir, True)

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:334: def PrepareShellRun(self, origin=None,
install=True):
nit: The class member |shell_apk_path| isn't used elsewhere afaict. Maybe remove
that and change |install| to an optional |apk_to_install=None|.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:357:
subprocess.check_call(self._CreateADBCommand(['forward', 'tcp:5039',
nit: Could this be inlined in (or called from) StartShell?

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:370: def _GetLocalGdbPath(self):
nit: inline in caller?

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:380: pid = self._GetProcessId(process)
nit: maybe ensure this doesn't return 0?

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:386: atexit.register(_ExitIfNeeded,
gdbserver_process)
Interesting, will this do the right thing if the process is being run on the
device?

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:394: # Wait a second for gdb to start up on the
device.
Hmm, I wonder how reliable this is, and what the failure looks like...

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:486: 
nit: the rest of this file uses one blank line between functions, ditto below.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:493: gdb_path: path to gdbinit
nit: gdb_init_path

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:495: gdbinit = ('target remote localhost:5039\n'
How odd, I've never seen this before...

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:525: 'html_viewer.mojo': ['libhtml_viewer',
'html_viewer_library.so']
nit: trailing comma

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:532: os.symlink(src, os.path.join(dest_dir,
android_name))
Do we need to clean these up at all?

[sky, 2015-05-22 17:27:24]

I also fixed a problem with handling of control-c.

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
File mandoline/tools/android_run_mandoline.py (right):

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
mandoline/tools/android_run_mandoline.py:33:
parser.add_argument('--dont-install',
On 2015/05/22 02:41:43, msw wrote:
> q: just curious, is this needed or just for efficiency?

For efficiency. If you're doing a bunch of debugging cycles without rebuilding
than waiting for the install is a pain!

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
mandoline/tools/android_run_mandoline.py:59: shell.StartShell(args, sys.stdout,
p.terminate, gdb=launcher_args.gdb)
On 2015/05/22 02:41:43, msw wrote:
> nit: you don't actually need "gdb=" here

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/android_mojo_she...
File mojo/tools/android_mojo_shell.py (right):

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/android_mojo_she...
mojo/tools/android_mojo_shell.py:39: launcher_args.target_device,
src_root=paths.src_root)
On 2015/05/22 02:41:43, msw wrote:
> nit: not needed, or maybe add the remaining gdb support bits here?

I nuked the src_root from here, as you say it isn't needed. If we find it would
be nice to have gdb for mojo_shell too, we can add it.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py
File mojo/tools/mopy/android.py (right):

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:184: class Tmpdir(object):
On 2015/05/22 02:41:43, msw wrote:
> nit: Maybe just inline this in _WaitForProcessIdAndStartGdb:
>   temp_dir = tempfile.mkdtemp()
>   atexit.register(shutil.rmtree, temp_dir, True)

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:334: def PrepareShellRun(self, origin=None,
install=True):
On 2015/05/22 02:41:43, msw wrote:
> nit: The class member |shell_apk_path| isn't used elsewhere afaict. Maybe
remove
> that and change |install| to an optional |apk_to_install=None|.

That would make the call sites more complex. I left this as is.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:357:
subprocess.check_call(self._CreateADBCommand(['forward', 'tcp:5039',
On 2015/05/22 02:41:44, msw wrote:
> nit: Could this be inlined in (or called from) StartShell?

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:370: def _GetLocalGdbPath(self):
On 2015/05/22 02:41:43, msw wrote:
> nit: inline in caller?

IMO the separate function makes this more readable.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:380: pid = self._GetProcessId(process)
On 2015/05/22 02:41:44, msw wrote:
> nit: maybe ensure this doesn't return 0?

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:386: atexit.register(_ExitIfNeeded,
gdbserver_process)
On 2015/05/22 02:41:43, msw wrote:
> Interesting, will this do the right thing if the process is being run on the
> device?

Earlier code makes sure the app isn't already running.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:394: # Wait a second for gdb to start up on the
device.
On 2015/05/22 02:41:43, msw wrote:
> Hmm, I wonder how reliable this is, and what the failure looks like...

No doubt it's flakey. I added a TODO to try a couple of times and then give up.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:486: 
On 2015/05/22 02:41:43, msw wrote:
> nit: the rest of this file uses one blank line between functions, ditto below.

That is true for class functions, but not top level functions (see line 163).

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:493: gdb_path: path to gdbinit
On 2015/05/22 02:41:43, msw wrote:
> nit: gdb_init_path

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:525: 'html_viewer.mojo': ['libhtml_viewer',
'html_viewer_library.so']
On 2015/05/22 02:41:44, msw wrote:
> nit: trailing comma

Done.

https://codereview.chromium.org/1152663002/diff/1/mojo/tools/mopy/android.py#...
mojo/tools/mopy/android.py:532: os.symlink(src, os.path.join(dest_dir,
android_name))
On 2015/05/22 02:41:43, msw wrote:
> Do we need to clean these up at all?

This is put in the directory that is removed by shutil.rmtree.

[msw, 2015-05-22 17:52:52]

lgtm

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
File mandoline/tools/android_run_mandoline.py (right):

https://codereview.chromium.org/1152663002/diff/1/mandoline/tools/android_run...
mandoline/tools/android_run_mandoline.py:33:
parser.add_argument('--dont-install',
On 2015/05/22 17:27:24, sky wrote:
> On 2015/05/22 02:41:43, msw wrote:
> > q: just curious, is this needed or just for efficiency?
> 
> For efficiency. If you're doing a bunch of debugging cycles without rebuilding
> than waiting for the install is a pain!

This may not be needed after Jay's CL to install as needed:
  http://codereview.chromium.org/1144703004

[sky, 2015-05-22 18:03:01]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2015-05-22 18:03:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152663002/40001

[commit-bot: I haz the power, 2015-05-22 19:17:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-22 19:18:42]

Patchset 3 (id:??) landed as
https://crrev.com/2c7d24cd78f53c232d0c80585e8129f854af1f43
Cr-Commit-Position: refs/heads/master@{#331146}