Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1209593002:
GDB support for Android in devtools' debugger. (Closed)
|
Created:
5 years, 6 months ago by etiennej Modified:
5 years, 5 months ago CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
git@github.com:domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionGDB support for Android in devtools' debugger.
See README.md for usage.
R=ppi@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/72098cf04f5ecf30c9f567bf4ece19a46ba6ba9d
Patch Set 1 #
Total comments: 62
Patch Set 2 : #Patch Set 3 : #
Total comments: 78
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Kill stray remote file reader #
Total comments: 32
Patch Set 7 : #
Total comments: 5
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 18 (1 generated)
etiennej@chromium.org changed reviewers: + ppi@chromium.org, qsr@chromium.org
Sweet! Please add instructions for this in https://github.com/domokit/mojo/blob/master/README.md#debugging-tracing-profi... . Please find first-pass comments below. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/config.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:5: # Path where remote_file_reader should be installed on the device End with a period. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:8: # Path where remote_file_reader is available on Cloud Storage End with a period. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:10: Remove this blank line. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/install_remote_file_reader.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/install_remote_file_reader.py:8: The style guide wants two blank lines here I think. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:5: import os.path This seems not to be used. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:13: Two blank lines please. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:16: remote device.""" The closing triple quote should be on the next line (unless all of the pydoc fits on one line). https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:35: def seek(self, pos, mode = 0): Remove spaces around "=". https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:66: Remove the blank line. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:10: import re Unused. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:12: import socket Unused? https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:13: import struct Unused. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:36: """Execute a GDB command.""" s/Execute/Executes/ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:74: sys.path.append(os.path.join(third_party_dir, 'pyelftools')) devtools are meant to be consumed in other repositories, too. It's fine to try to use the one in third party, but add a check if this succeeded, and if not, print an informative error message. (Couldn't import pyelftools, run `pip install ...` if you want to install it.) https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:102: Single blank lines between class members, please. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:111: result = 16 * [ 0 ] Remove spaces around "0". https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:115: data[i:i+len(result)], Add spaces around "+". https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:127: """Download a remote file through GDB connection. s/Download/Downloads/ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:129: Returns the filename The format is: """ Returns: What is being returned """ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:168: """Update the mapping between symbols as seen from GDB and local library s/Update/Updates/ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:204: Just one blank line. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:206: """Get the PID of an application running on a device.""" s/Get/Gets/ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:215: """Start a debugging session.""" s/Start/Starts/ https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:240: Remove the trailing blank lines. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugger File mojo/devtools/common/debugger (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:11: import tempfile add a blank line https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:152: ndk_dir = os.path.join(_GetDirAbove('third_party'), 'third_party', We already have similar logic in devtools, in `stack`. We should probably move these two here: def _LowestAncestorContainingRelpath(dir_path, relpath): def _GuessDir(relpath): , and pass already inferred paths to `stack`. We can do it in a separate CL if you prefer. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:177: # We need to pass some commands to GDB at startup End with a period. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:187: debug_session_arguments = ', '.join([k + '=' + string_escape(v) for Where string_escape is defined or imported? https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:189: gdb_setup_commands.write('py d = DebugSession('+debug_session_arguments+')\n') Add spaced around "+"es. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:218: help='debug a shell instance currently running on a device') "debug" doesn't say much. Please clarify that we are attaching to a shell already running under gdb, or whatever else exactly it is that we are doing.
https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugger File mojo/devtools/common/debugger (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:225: device_gdb_parser.set_defaults(func=_device_gdb) Hum. It would be good if that worked also for desktop. I think we should have an API to use gdb, and the specifics should go in a library with one implementation for desktop and one implementation of android. The same way we do for launching the shell.
https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/config.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:5: # Path where remote_file_reader should be installed on the device On 2015/06/24 15:14:07, ppi wrote: > End with a period. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:8: # Path where remote_file_reader is available on Cloud Storage On 2015/06/24 15:14:07, ppi wrote: > End with a period. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/config.py:10: On 2015/06/24 15:14:07, ppi wrote: > Remove this blank line. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/install_remote_file_reader.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/install_remote_file_reader.py:8: On 2015/06/24 15:14:07, ppi wrote: > The style guide wants two blank lines here I think. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:5: import os.path On 2015/06/24 15:14:07, ppi wrote: > This seems not to be used. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:13: On 2015/06/24 15:14:07, ppi wrote: > Two blank lines please. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:16: remote device.""" On 2015/06/24 15:14:07, ppi wrote: > The closing triple quote should be on the next line (unless all of the pydoc > fits on one line). Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:35: def seek(self, pos, mode = 0): On 2015/06/24 15:14:07, ppi wrote: > Remove spaces around "=". Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/remote_file_connection.py:66: On 2015/06/24 15:14:07, ppi wrote: > Remove the blank line. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:10: import re On 2015/06/24 15:14:07, ppi wrote: > Unused. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:12: import socket On 2015/06/24 15:14:08, ppi wrote: > Unused? Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:13: import struct On 2015/06/24 15:14:08, ppi wrote: > Unused. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:36: """Execute a GDB command.""" On 2015/06/24 15:14:08, ppi wrote: > s/Execute/Executes/ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:74: sys.path.append(os.path.join(third_party_dir, 'pyelftools')) On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > devtools are meant to be consumed in other repositories, too. It's fine to try > to use the one in third party, but add a check if this succeeded, and if not, > print an informative error message. (Couldn't import pyelftools, run `pip > install ...` if you want to install it.) Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:102: On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Single blank lines between class members, please. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:111: result = 16 * [ 0 ] On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Remove spaces around "0". Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:115: data[i:i+len(result)], On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Add spaces around "+". Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:127: """Download a remote file through GDB connection. On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > s/Download/Downloads/ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:129: Returns the filename On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > The format is: > """ > Returns: > What is being returned > """ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:168: """Update the mapping between symbols as seen from GDB and local library On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > s/Update/Updates/ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:204: On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Just one blank line. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:206: """Get the PID of an application running on a device.""" On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > s/Get/Gets/ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:215: """Start a debugging session.""" On 2015/06/24 15:14:07, ppi (ooo until 8th June) wrote: > s/Start/Starts/ Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/androi... mojo/devtools/common/android_gdb/session.py:240: On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Remove the trailing blank lines. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugger File mojo/devtools/common/debugger (right): https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:11: import tempfile On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > add a blank line Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:152: ndk_dir = os.path.join(_GetDirAbove('third_party'), 'third_party', On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > We already have similar logic in devtools, in `stack`. We should probably move > these two here: > > def _LowestAncestorContainingRelpath(dir_path, relpath): > def _GuessDir(relpath): > > , and pass already inferred paths to `stack`. > > We can do it in a separate CL if you prefer. Added a TODO. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:177: # We need to pass some commands to GDB at startup On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > End with a period. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:187: debug_session_arguments = ', '.join([k + '=' + string_escape(v) for On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Where string_escape is defined or imported? Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:189: gdb_setup_commands.write('py d = DebugSession('+debug_session_arguments+')\n') On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > Add spaced around "+"es. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:218: help='debug a shell instance currently running on a device') On 2015/06/24 15:14:08, ppi (ooo until 8th June) wrote: > "debug" doesn't say much. Please clarify that we are attaching to a shell > already running under gdb, or whatever else exactly it is that we are doing. Done. https://codereview.chromium.org/1209593002/diff/1/mojo/devtools/common/debugg... mojo/devtools/common/debugger:225: device_gdb_parser.set_defaults(func=_device_gdb) On 2015/06/24 15:29:01, qsr wrote: > Hum. It would be good if that worked also for desktop. I think we should have an > API to use gdb, and the specifics should go in a library with one implementation > for desktop and one implementation of android. The same way we do for launching > the shell. My understanding is that ppi@ wanted to have all android-related tools under the same switch ("device"), and I am sharing a number of flags with "device stack". However, I would be happy to move the flags and switches elsewhere. ppi@: what do you think?
ppi: I addressed your offline comments.
https://codereview.chromium.org/1209593002/diff/40001/README.md File README.md (right): https://codereview.chromium.org/1209593002/diff/40001/README.md#newcode268 README.md:268: #### GDB support "support" is redundant, let's just say "gdb" or "Attaching gdb"? https://codereview.chromium.org/1209593002/diff/40001/README.md#newcode273 README.md:273: mojo/devtools/common/debugger gdb attach I just tried to do just that, and the process hanged: $ mojo/devtools/common/debugger gdb attach ^CTraceback (most recent call last): File "mojo/devtools/common/debugger", line 259, in <module> sys.exit(main()) File "mojo/devtools/common/debugger", line 256, in main return args.func(args) File "mojo/devtools/common/debugger", line 162, in _gdb_attach _GetDirAbove('depot_tools'), 'depot_tools', 'third_party', 'gsutil', File "mojo/devtools/common/debugger", line 26, in _GetDirAbove if dirname in os.listdir(path): KeyboardInterrupt There are two action items here: - could you make it work out of the box in the regular mojo checkout? - we probably want to have an explicit error message instead of infinite loop if some directory / tool cannot be located https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/install_remote_file_reader.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/install_remote_file_reader.py:10: def install_binary(gsutil, adb='adb'): install_binary -> install_remote_file_reader https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:22: self._size_struct = struct.Struct("!i") is "size_struct" the only private member? Should all of them get the underscores? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:27: def close(self): if this is a pair for "connect", let's call it "disconnect", or something else that does not look like a pair for "open" below. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:30: def open(self, filename): When the file will be closed on the remote side? Is it ok to call open multiple times for different files? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:40: raise RemoteFileConnectionException("Unable to open file.") Update the exception message here and below. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:55: raise RemoteFileConnectionException("socket connection broken") Capitalize and end with a period to match the others, here and below. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:4: please add a top-level pydoc explaining what this module does (ie. that we use it to script gdb, and what is the goal of that). (we could list/explain here the key gdb commands we're issuing, as a high-level guide to all this black magic - wdyt?) https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:17: FILE = __file__ Why? __file__ is a known thing, should we stick with it? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:24: def _GetDirAbove(dirname): Let's make this return None if the dir is not found, instead of getting into an infinite loop. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:24: def _GetDirAbove(dirname): _get_dir_above https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:24: def _GetDirAbove(dirname): Can you infer all paths in `debugger` and always pass them to the DebugSession, so that we don't have this get_dir_above both here and there? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:34: def gdb_execute(command): _gdb_execute? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:39: class Mapping(object): Please add a pydoc - what does a "Mapping" represent? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:48: def get_mapped_files(): _get_mapped_files? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:48: def get_mapped_files(): Please describe the output format / value. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:52: if len(x) == 5 and x[4][0] == '/'] Please explain what's happening here - example return value from `info proc map` would be helpful. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:63: package_name='org.chromium.mojo.shell', Let's push this default up to `debugger`? It's likely that this won't be the only tool for which the shell package name will matter. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:98: self._elffile = elffile _elffile_module? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:100: self.libraries = self._find_libraries(build_directory) unify the naming convention, should all of these be _libraries, _rfc, etc.? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:115: def _find_libraries(self, lib_dir): Please add a pydoc explaining the return value. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:124: def _get_signature(self, file_object): file_object -> file? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:125: """Compute a unique signature of a library file.""" Computes https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:129: data = file_object.read(min(4096, text_section['sh_size'])) Are we hashing only the first 4096 bytes of the text section? Why? Can we just hash the entire file without using elffile? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:165: for s in m: what is 's'? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:180: else: Can you comment on when we hit this case and what we're doing there? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:195: # Map all symbols in /data/ Which are the symbols in "/data"? Are these the native libraries packaged with the apk? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:226: def _get_pid(self, application): _get_device_app_pid? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:235: def start_debug(self): The class is already called DebugSession, maybe just "start" ? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:246: time.sleep(0.1) Why? Please explain the sleep in a comment. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:248: read_process = subprocess.Popen( reader_process? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... File mojo/devtools/common/debugger (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:20: def _GetDirAbove(dirname): _get_dir_above, let's make it not loop infinitely when the dir is not found https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:181: gdb_setup_commands = tempfile.NamedTemporaryFile() maybe 'gdb_commands_file'? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:190: debug_session_arguments = ', '.join( Better to use a new variable / not to change the type of an existing one. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:190: debug_session_arguments = ', '.join( Maybe compute this string before gdb command file is created, so that the file is written in one chunk of code, not intermixed with building the session arg dictionary?
https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/install_remote_file_reader.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/install_remote_file_reader.py:10: def install_binary(gsutil, adb='adb'): On 2015/07/09 13:12:38, ppi (ooo until 8th June) wrote: > install_binary -> install_remote_file_reader Given the name of the python package, what about just install ? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:27: def close(self): On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > if this is a pair for "connect", let's call it "disconnect", or something else > that does not look like a pair for "open" below. It has to be called close (and is in fact a pair for "open", as this is implementing the file interface. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:30: def open(self, filename): On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > When the file will be closed on the remote side? Is it ok to call open multiple > times for different files? The file will be closed when you disconnect, or when you open a new one. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:129: data = file_object.read(min(4096, text_section['sh_size'])) On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > Are we hashing only the first 4096 bytes of the text section? Why? > > Can we just hash the entire file without using elffile? You cannot hash the entire file, because on the host you have the file wiht debug information and on the device you have the stripped file. We only use 4096 bytes because this is enough and otherwise we would need to pass multiple Mo of data from the device to the host, which is slow. This scheme is what breakpad does to identity libraries. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:195: # Map all symbols in /data/ On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > Which are the symbols in "/data"? Are these the native libraries packaged with > the apk? Yes.
https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:27: def close(self): On 2015/07/09 13:54:18, qsr wrote: > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > if this is a pair for "connect", let's call it "disconnect", or something else > > that does not look like a pair for "open" below. > > It has to be called close (and is in fact a pair for "open", as this is > implementing the file interface. Ok, I was mis-remembering, we do not ever close this, so it can be renamed disconnect. What we should also do is add a __del__(self) method and call disconnect on it.
https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:129: data = file_object.read(min(4096, text_section['sh_size'])) On 2015/07/09 13:54:18, qsr wrote: > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > Are we hashing only the first 4096 bytes of the text section? Why? > > > > Can we just hash the entire file without using elffile? > > You cannot hash the entire file, because on the host you have the file wiht > debug information and on the device you have the stripped file. > > We only use 4096 bytes because this is enough and otherwise we would need to > pass multiple Mo of data from the device to the host, which is slow. > > This scheme is what breakpad does to identity libraries. Makes sense. Etienne, could you please import these explanation as a comment? https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:195: # Map all symbols in /data/ On 2015/07/09 13:54:18, qsr wrote: > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > Which are the symbols in "/data"? Are these the native libraries packaged with > > the apk? > > Yes. Thanks! I guess I meant that this could be said in the comment:).
I am still working on properly passing Ctrl-C down to gdb. https://codereview.chromium.org/1209593002/diff/40001/README.md File README.md (right): https://codereview.chromium.org/1209593002/diff/40001/README.md#newcode268 README.md:268: #### GDB support On 2015/07/09 13:12:38, ppi (ooo until 15th June) wrote: > "support" is redundant, let's just say "gdb" or "Attaching gdb"? Done. https://codereview.chromium.org/1209593002/diff/40001/README.md#newcode273 README.md:273: mojo/devtools/common/debugger gdb attach On 2015/07/09 13:12:38, ppi (ooo until 15th June) wrote: > I just tried to do just that, and the process hanged: > > $ mojo/devtools/common/debugger gdb attach > > > ^CTraceback (most recent call last): > File "mojo/devtools/common/debugger", line 259, in <module> > sys.exit(main()) > File "mojo/devtools/common/debugger", line 256, in main > return args.func(args) > File "mojo/devtools/common/debugger", line 162, in _gdb_attach > _GetDirAbove('depot_tools'), 'depot_tools', 'third_party', 'gsutil', > File "mojo/devtools/common/debugger", line 26, in _GetDirAbove > if dirname in os.listdir(path): > KeyboardInterrupt > > > There are two action items here: > > - could you make it work out of the box in the regular mojo checkout? > - we probably want to have an explicit error message instead of infinite loop > if some directory / tool cannot be located I don't know how your "regular mojo checkout" differs from mine, it might be worth investigating. In any case, we now have an explicit error message and no more infinite loop. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/install_remote_file_reader.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/install_remote_file_reader.py:10: def install_binary(gsutil, adb='adb'): On 2015/07/09 13:54:18, qsr wrote: > On 2015/07/09 13:12:38, ppi (ooo until 8th June) wrote: > > install_binary -> install_remote_file_reader > > Given the name of the python package, what about just install ? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/remote_file_connection.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:22: self._size_struct = struct.Struct("!i") On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > is "size_struct" the only private member? Should all of them get the > underscores? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:27: def close(self): On 2015/07/09 14:06:16, qsr wrote: > On 2015/07/09 13:54:18, qsr wrote: > > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > > if this is a pair for "connect", let's call it "disconnect", or something > else > > > that does not look like a pair for "open" below. > > > > It has to be called close (and is in fact a pair for "open", as this is > > implementing the file interface. > > Ok, I was mis-remembering, we do not ever close this, so it can be renamed > disconnect. What we should also do is add a __del__(self) method and call > disconnect on it. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:30: def open(self, filename): On 2015/07/09 13:54:18, qsr wrote: > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > When the file will be closed on the remote side? Is it ok to call open > multiple > > times for different files? > > The file will be closed when you disconnect, or when you open a new one. Acknowledged. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:40: raise RemoteFileConnectionException("Unable to open file.") On 2015/07/09 13:12:38, ppi (ooo until 15th June) wrote: > Update the exception message here and below. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/remote_file_connection.py:55: raise RemoteFileConnectionException("socket connection broken") On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Capitalize and end with a period to match the others, here and below. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:4: On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > please add a top-level pydoc explaining what this module does (ie. that we use > it to script gdb, and what is the goal of that). > > (we could list/explain here the key gdb commands we're issuing, as a high-level > guide to all this black magic - wdyt?) I gave a high-level description, and put the details into |DebugSession| documentation. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:17: FILE = __file__ On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Why? __file__ is a known thing, should we stick with it? There was an issue with __file__ not being defined after the import phase. As we are providing all the paths to DebugSession (instead of having it guess), we no longer need it. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:24: def _GetDirAbove(dirname): On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Can you infer all paths in `debugger` and always pass them to the DebugSession, > so that we don't have this get_dir_above both here and there? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:34: def gdb_execute(command): On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > _gdb_execute? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:39: class Mapping(object): On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Please add a pydoc - what does a "Mapping" represent? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:48: def get_mapped_files(): On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > _get_mapped_files? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:48: def get_mapped_files(): On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Please describe the output format / value. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:52: if len(x) == 5 and x[4][0] == '/'] On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Please explain what's happening here - example return value from `info proc map` > would be helpful. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:63: package_name='org.chromium.mojo.shell', On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > Let's push this default up to `debugger`? It's likely that this won't be the > only tool for which the shell package name will matter. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:98: self._elffile = elffile On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > _elffile_module? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:100: self.libraries = self._find_libraries(build_directory) On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > unify the naming convention, should all of these be _libraries, _rfc, etc.? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:115: def _find_libraries(self, lib_dir): On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Please add a pydoc explaining the return value. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:124: def _get_signature(self, file_object): On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > file_object -> file? I would prefer not redefining python internals. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:125: """Compute a unique signature of a library file.""" On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Computes Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:129: data = file_object.read(min(4096, text_section['sh_size'])) On 2015/07/09 14:16:11, ppi (ooo until 15th June) wrote: > On 2015/07/09 13:54:18, qsr wrote: > > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > > Are we hashing only the first 4096 bytes of the text section? Why? > > > > > > Can we just hash the entire file without using elffile? > > > > You cannot hash the entire file, because on the host you have the file wiht > > debug information and on the device you have the stripped file. > > > > We only use 4096 bytes because this is enough and otherwise we would need to > > pass multiple Mo of data from the device to the host, which is slow. > > > > This scheme is what breakpad does to identity libraries. > > Makes sense. Etienne, could you please import these explanation as a comment? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:165: for s in m: On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > what is 's'? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:180: else: On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Can you comment on when we hit this case and what we're doing there? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:195: # Map all symbols in /data/ On 2015/07/09 14:16:10, ppi (ooo until 15th June) wrote: > On 2015/07/09 13:54:18, qsr wrote: > > On 2015/07/09 13:12:39, ppi (ooo until 8th June) wrote: > > > Which are the symbols in "/data"? Are these the native libraries packaged > with > > > the apk? > > > > Yes. > > Thanks! I guess I meant that this could be said in the comment:). Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:226: def _get_pid(self, application): On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > _get_device_app_pid? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:235: def start_debug(self): On 2015/07/09 13:12:39, ppi (ooo until 15th June) wrote: > The class is already called DebugSession, maybe just "start" ? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:246: time.sleep(0.1) On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Why? Please explain the sleep in a comment. Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/an... mojo/devtools/common/android_gdb/session.py:248: read_process = subprocess.Popen( On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > reader_process? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... File mojo/devtools/common/debugger (right): https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:20: def _GetDirAbove(dirname): On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > _get_dir_above, let's make it not loop infinitely when the dir is not found Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:181: gdb_setup_commands = tempfile.NamedTemporaryFile() On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > maybe 'gdb_commands_file'? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:190: debug_session_arguments = ', '.join( On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Maybe compute this string before gdb command file is created, so that the file > is written in one chunk of code, not intermixed with building the session arg > dictionary? Done. https://codereview.chromium.org/1209593002/diff/40001/mojo/devtools/common/de... mojo/devtools/common/debugger:190: debug_session_arguments = ', '.join( On 2015/07/09 13:12:40, ppi (ooo until 15th June) wrote: > Better to use a new variable / not to change the type of an existing one. If one cannot redefine its variable types in Python anymore, ... :) Done.
I fixed the signal handling. ppi@, ptal
https://codereview.chromium.org/1209593002/diff/100001/README.md File README.md (right): https://codereview.chromium.org/1209593002/diff/100001/README.md#newcode273 README.md:273: mojo/devtools/common/debugger gdb attach The error I get is "CRITICAL:root:Unable to find gsutil, please specify its pathwith --gsutil-dir." This is clearly an improvement:). I think you assume that depot_tools is somewhere above the user checkout. Can you do something along the lines of find_depot_tolls.py instead? https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:3: # found in the LICENSE file. Add a blank line after the license. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:26: import time Unused? https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:33: Two blank lines around top-level definitions please. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:37: Here too. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:38: def get_signature(file_object, elffile_module): Is it used outside this module? If not, _get_signature please. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:61: """Mapping represents a mapped memory region.""" s/Mapping represents/Represents/ https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:71: """Retrieve all the files mapped into the debugged process memory. Retrieves https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:74: List of mapped memory regions grouped by files. Just two leading spaces. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:77: # start address, end address, size, offset, file path End comment with a period. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:89: class DebugSession(object): This now specific to a remote gdb server on the device, but substantial amount of the logic seems to be applicable to running on Linux, and we might want to re-use this code in the future. Wdyt about renaming the directory "android_gdb" -> "gdb"? I imagine we wouldn't do "android_gdb" and "linux_gdb" but rather have one library. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:100: Too many blank lines. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:122: # guaranteed to be called when the interpreter (GDB, in our case) quits. Should we then install an at-exit handler? https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:125: if self._remote_file_reader_process != None: maybe "!= None" --> "is not None" https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:228: except gdb.error as _: do we need "as _"? https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:229: import traceback All imports at the top please.
https://codereview.chromium.org/1209593002/diff/100001/README.md File README.md (right): https://codereview.chromium.org/1209593002/diff/100001/README.md#newcode273 README.md:273: mojo/devtools/common/debugger gdb attach On 2015/07/15 13:54:18, ppi wrote: > The error I get is > > "CRITICAL:root:Unable to find gsutil, please specify its pathwith --gsutil-dir." > > This is clearly an improvement:). > > I think you assume that depot_tools is somewhere above the user checkout. Can > you do something along the lines of find_depot_tolls.py instead? Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:3: # found in the LICENSE file. On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Add a blank line after the license. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:26: import time On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Unused? Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:33: On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Two blank lines around top-level definitions please. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:37: On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Here too. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:38: def get_signature(file_object, elffile_module): On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Is it used outside this module? If not, _get_signature please. qsr@ wants to use it elsewhere, yes. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:61: """Mapping represents a mapped memory region.""" On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > s/Mapping represents/Represents/ Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:71: """Retrieve all the files mapped into the debugged process memory. On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Retrieves Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:74: List of mapped memory regions grouped by files. On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Just two leading spaces. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:77: # start address, end address, size, offset, file path On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > End comment with a period. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:89: class DebugSession(object): On 2015/07/15 13:54:18, ppi wrote: > This now specific to a remote gdb server on the device, but substantial amount > of the logic seems to be applicable to running on Linux, and we might want to > re-use this code in the future. > > Wdyt about renaming the directory "android_gdb" -> "gdb"? I imagine we wouldn't > do "android_gdb" and "linux_gdb" but rather have one library. Given the amount of android-specific stuff compared to generic code, I feel that removing "android" would be making false promises. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:100: On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Too many blank lines. Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:122: # guaranteed to be called when the interpreter (GDB, in our case) quits. On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > Should we then install an at-exit handler? Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:125: if self._remote_file_reader_process != None: On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > maybe "!= None" --> "is not None" Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:228: except gdb.error as _: On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > do we need "as _"? Done. https://codereview.chromium.org/1209593002/diff/100001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:229: import traceback On 2015/07/15 13:54:18, ppi (ooo until 15th June) wrote: > All imports at the top please. Done.
lgtm, thanks! https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:129: Remove the trailing whitespace (four spaces). https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:130: def stop(self, _ = None): No spaces around "=". https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:130: def stop(self, _ = None): I'd probably call the second variable _unused_return_value to make the code less puzzling, and then suppress the pylint check, maybe it's not worth it though.
https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... File mojo/devtools/common/android_gdb/session.py (right): https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:129: On 2015/07/16 09:28:19, ppi wrote: > Remove the trailing whitespace (four spaces). Done. https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... mojo/devtools/common/android_gdb/session.py:130: def stop(self, _ = None): On 2015/07/16 09:28:19, ppi wrote: > No spaces around "=". Done.
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as 72098cf04f5ecf30c9f567bf4ece19a46ba6ba9d (presubmit successful).
Message was sent while issue was closed.
On 2015/07/16 09:28:19, ppi wrote: > lgtm, thanks! > > https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... > File mojo/devtools/common/android_gdb/session.py (right): > > https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... > mojo/devtools/common/android_gdb/session.py:129: > Remove the trailing whitespace (four spaces). > > https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... > mojo/devtools/common/android_gdb/session.py:130: def stop(self, _ = None): > No spaces around "=". > > https://codereview.chromium.org/1209593002/diff/120001/mojo/devtools/common/a... > mojo/devtools/common/android_gdb/session.py:130: def stop(self, _ = None): > I'd probably call the second variable _unused_return_value to make the code less > puzzling, and then suppress the pylint check, maybe it's not worth it though. Is there a tool a la clangfmt that can do this formatting automatically? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
