Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
891743003:
Make it possible to debug Release builds on rooted phones (Closed)
|
Created:
5 years, 10 months ago by mille Modified:
5 years, 10 months ago Reviewers:
jbudorick CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@CHR-3757 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake it possible to debug Release builds on rooted phones
https://codereview.chromium.org/884993003 broke gdb debugging
on Release buils. This commit makes sure it works to debug
Debug and Release (only on rooted device) builds.
BUG=453379
Committed: https://crrev.com/bb4db81dcbbe045c9ed2a7d05662af06064a3884
Cr-Commit-Position: refs/heads/master@{#314567}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Don't assume COMMAND_PREFIX=run as PACKAGE_NAME #
Total comments: 2
Patch Set 3 : Remove unnecessary conditional #Messages
Total messages: 16 (2 generated)
jimmym@opera.com changed reviewers: + jbudorick@chromium.org
Follow up to https://codereview.chromium.org/884993003/
https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb File build/android/adb_gdb (left): https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp $TMP_TARGET_GDBSERVER . What's wrong with just changing this line to adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER
https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb File build/android/adb_gdb (left): https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp $TMP_TARGET_GDBSERVER . On 2015/01/30 15:43:17, jbudorick wrote: > What's wrong with just changing this line to > > adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER There is nothing wrong with doing that. I guess that would be better since COMMAND_PREFIX can be empty which would mean we would copy it to the root directory, I guess.
On 2015/01/30 16:11:59, mille wrote: > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb > File build/android/adb_gdb (left): > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... > build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp > $TMP_TARGET_GDBSERVER . > On 2015/01/30 15:43:17, jbudorick wrote: > > What's wrong with just changing this line to > > > > adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER > > There is nothing wrong with doing that. I guess that would be better since > COMMAND_PREFIX can be empty which would mean we would copy it to the root > directory, I guess. note that mine had $TARGET_GDBSERVER as the second argument to cp, not ".", so it'd still be copied to /data/data/...
On 2015/01/30 16:13:41, jbudorick wrote: > On 2015/01/30 16:11:59, mille wrote: > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb > > File build/android/adb_gdb (left): > > > > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... > > build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp > > $TMP_TARGET_GDBSERVER . > > On 2015/01/30 15:43:17, jbudorick wrote: > > > What's wrong with just changing this line to > > > > > > adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER > > > > There is nothing wrong with doing that. I guess that would be better since > > COMMAND_PREFIX can be empty which would mean we would copy it to the root > > directory, I guess. > > note that mine had $TARGET_GDBSERVER as the second argument to cp, not ".", so > it'd still be copied to /data/data/... I meant that if we used "." and COMMAND_PREFIX was empty we would end up copying the file to the root. Your way is will always copy to /data/data/...
https://codereview.chromium.org/891743003/diff/20001/build/android/adb_gdb File build/android/adb_gdb (left): https://codereview.chromium.org/891743003/diff/20001/build/android/adb_gdb#ol... build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp $TMP_TARGET_GDBSERVER . I should've been more clear: why doesn't this CL _only_ use adb shell $COMMAND_PREFIX cp here instead of adding the if/else and renaming TMP_TARGET_GDBSERVER to ROOT_TARGET_GDBSERVER?
On 2015/01/30 16:39:07, mille wrote: > On 2015/01/30 16:13:41, jbudorick wrote: > > On 2015/01/30 16:11:59, mille wrote: > > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb > > > File build/android/adb_gdb (left): > > > > > > > > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... > > > build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp > > > $TMP_TARGET_GDBSERVER . > > > On 2015/01/30 15:43:17, jbudorick wrote: > > > > What's wrong with just changing this line to > > > > > > > > adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER > > > > > > There is nothing wrong with doing that. I guess that would be better since > > > COMMAND_PREFIX can be empty which would mean we would copy it to the root > > > directory, I guess. > > > > note that mine had $TARGET_GDBSERVER as the second argument to cp, not ".", so > > it'd still be copied to /data/data/... > > I meant that if we used "." and COMMAND_PREFIX was empty we would end up copying > the file to the root. Your way is will always copy to /data/data/... Yeah, I mistakenly thought you had missed that in the suggestion. Apologies.
On 2015/01/30 16:44:03, jbudorick wrote: > On 2015/01/30 16:39:07, mille wrote: > > On 2015/01/30 16:13:41, jbudorick wrote: > > > On 2015/01/30 16:11:59, mille wrote: > > > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb > > > > File build/android/adb_gdb (left): > > > > > > > > > > > > > > https://codereview.chromium.org/891743003/diff/1/build/android/adb_gdb#oldcod... > > > > build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp > > > > $TMP_TARGET_GDBSERVER . > > > > On 2015/01/30 15:43:17, jbudorick wrote: > > > > > What's wrong with just changing this line to > > > > > > > > > > adb shell $COMMAND_PREFIX cp $TMP_TARGET_GDBSERVER $TARGET_GDBSERVER > > > > > > > > There is nothing wrong with doing that. I guess that would be better since > > > > COMMAND_PREFIX can be empty which would mean we would copy it to the root > > > > directory, I guess. > > > > > > note that mine had $TARGET_GDBSERVER as the second argument to cp, not ".", > so > > > it'd still be copied to /data/data/... > > > > I meant that if we used "." and COMMAND_PREFIX was empty we would end up > copying > > the file to the root. Your way is will always copy to /data/data/... > > Yeah, I mistakenly thought you had missed that in the suggestion. Apologies. Did you see the patch or were there something else that bothered you?
https://codereview.chromium.org/891743003/diff/20001/build/android/adb_gdb File build/android/adb_gdb (left): https://codereview.chromium.org/891743003/diff/20001/build/android/adb_gdb#ol... build/android/adb_gdb:911: adb shell run-as $PACKAGE_NAME cp $TMP_TARGET_GDBSERVER . On 2015/01/30 16:43:23, jbudorick wrote: > I should've been more clear: why doesn't this CL _only_ use adb shell > $COMMAND_PREFIX cp here instead of adding the if/else and renaming > TMP_TARGET_GDBSERVER to ROOT_TARGET_GDBSERVER? I did see the patch. This was my comment.
lgtm
The CQ bit was checked by jimmym@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891743003/40001
On 2015/02/04 15:27:28, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/891743003/40001 Thanks for the patch, and sorry about the confusion.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bb4db81dcbbe045c9ed2a7d05662af06064a3884 Cr-Commit-Position: refs/heads/master@{#314567} |
