Closed Bug 906816 Opened 11 years ago Closed 11 years ago

Modify run-gdb.sh to attach by process name

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
A bit annoyed at having to run 'adb shell b2g-ps', find pid, run-gdb.sh attach so I patched run-gdb.sh to do what I want.
Attachment #792345 - Flags: review?(justin.lebar+bug)
Comment on attachment 792345 [details] [diff] [review]
patch

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

::: run-gdb.sh
@@ +18,4 @@
>  if [ "$1" = "attach"  -a  -n "$2" ] ; then
>     B2G_PID=$2
>     if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then
> +      B2G_PID=$(adb shell b2g-ps | sed -e "1d" | grep "^$2" | awk '{print $3}')

I'm concerned about needing sed/grep and awk and having this work on all platforms.

I think it would be better to add an option to b2g-info to take a process name and return a pid, and call that instead.
Comment on attachment 792345 [details] [diff] [review]
patch

Dave is a better person to review this.

I'm not sure b2g-info is on all phones by now, though.  For example, on Leo, we can only flash gecko and gaia, so you have to push b2g-info yourself.
Attachment #792345 - Flags: review?(justin.lebar+bug) → review?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #1)
> Comment on attachment 792345 [details] [diff] [review]
> patch
> 
> Review of attachment 792345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: run-gdb.sh
> @@ +18,4 @@
> >  if [ "$1" = "attach"  -a  -n "$2" ] ; then
> >     B2G_PID=$2
> >     if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then
> > +      B2G_PID=$(adb shell b2g-ps | sed -e "1d" | grep "^$2" | awk '{print $3}')
> 
> I'm concerned about needing sed/grep and awk and having this work on all
> platforms.
> 
> I think it would be better to add an option to b2g-info to take a process
> name and return a pid, and call that instead.

It's a developer script so worse case these users are no worse off. I'm looking into patching b2g-info.
So, actually,

toolbox ps NAME

will already do the "find a process by name" portion. This combines b2g-ps and the grep portion.

Then you could do:

B2G_PID=$(echo $(adb shell 'toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n $pid)'))

and then you don't need sed, grep or awk.
Actually the $(echo ) is unecessary. so this simplifies to:

B2G_PID=$(adb shell 'toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n $pid)')
All right - I think that this is my final answer :)

To allow for the process name to contain embedded spaces we need to surround $B2G_PID in double quotes and pass down single quotes to toolbox ps so that it gets a single arg (I tested using 'FM Radio')

B2G_PID=$(adb shell 'toolbox ps '"'$B2G_PID'"' | (read header; read user pid rest; echo -n $pid)')

If you want to talk on IRC about how shell quoting works (its a little different from C++) then let me know.

I tested this by creating a small script I called find-app-name.sh which contained:

#!/bin/bash
#set -x
B2G_PID=$1
B2G_PID=$(adb shell 'toolbox ps '"'$B2G_PID'"' | (read header; read user pid rest; echo -n $pid)')
echo B2G_PID=${B2G_PID}

And then ran:

./find-app-name.sh 'FM Radio'

(after launching FM Radio on the phone.
An alternative way of quoting this would be:

B2G_PID=$(adb shell "toolbox ps '$B2G_PID' | (read header; read user pid rest; echo -n \$pid)")
And after all that, I see that run-gdb.sh already has this pattern when trying to figure out the pid of gdbserver and another case where it figures out the pid of b2g.

So we should write a function (which would go near the top of the file):

get_pid_by_name() {
    echo $(adb shell "toolbox ps '$1' | (read header; read user pid rest; echo -n \$pid)") 
}

and then around line 15:

  GDBSERVER_PID=$(get_pid_by_name gdbserver)

And around line 28:

  B2G_PID=$(get_pid_by_name b2g)

and then for the attach case, do:

  B2G_PID=$(get_pid_by_name "$B2G_PID")

The double quotes are needed in the last case since B2G_PID may contain embedded spaces.
Attached patch patch (obsolete) — Splinter Review
Ahh yes, toolbox is already in the script so let's start using that.

I replace adb with $ADB to conform with the script. I also added a line:
'Found Camera PID: 762'. It's a bit redundant since will print the pid it's attaching to but I find it handy.
Assignee: nobody → bgirard
Attachment #792345 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #792345 - Flags: review?(dhylands)
Attachment #792425 - Flags: review?(dhylands)
Comment on attachment 792425 [details] [diff] [review]
patch

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

r=me with nits addressed

::: run-gdb.sh
@@ +1,5 @@
>  #!/bin/bash
>  #set -x
>  
> +get_pid_by_name() {
> +    echo $($ADB shell "toolbox ps '$1' | (read header; read user pid rest; echo -n \$pid)") 

nit: trailing space

@@ +20,4 @@
>  
>  GDB_PORT=$((10000 + $(id -u) % 50000))
>  if [ "$1" = "attach"  -a  -n "$2" ] ; then
> +   ATTACH_TARGET=$2

nit: I would move this assignment inside the if below, since ATTACH_TARGET is only needed inside that if, and I'd probably change it to be;

ATTACH_TARGET=$B2G_PID (rather than $2)

@@ +24,4 @@
>     B2G_PID=$2
>     if [ -z "$($ADB ls /proc/$B2G_PID)" ] ; then
> +      B2G_PID=$(get_pid_by_name "$B2G_PID")
> +      if [ -z $B2G_PID ] ; then

nit: It's common to surround vars in double quotes when using -z or -n (so that you pass an empty string rather than no arguments)

I see my current version of bash works either way, but things break down if you later need to combine with other operators.
Attachment #792425 - Flags: review?(dhylands) → review+
Attached patch patchSplinter Review
What's the landing procedure for this repo?
Attachment #792425 - Attachment is obsolete: true
Attachment #792446 - Flags: review+
Send a PR and I (or, probably, Dave) can merge it.
https://github.com/mozilla-b2g/B2G/commit/dcd6e13d2f119444624db62d0962715bc282e090
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: