977156 - Build vgdb on b2g and add support for it

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: b2g-vgdb.patch

This patch to base b2g scripts and external/valgrind causes vgdb to get built.  It can then (almost -- see below) be run with:

./run-valgrind.sh [nocopy] vgdb

[in another window]
[once] adb forward tcp:1234 tcp:1234
adb shell 'TMPDIR=/data/local/tmp LOGNAME=b2g HOSTNAME=b2g vgdb --port=1234'

[in another window]
./run-gdb.sh vgdb

At this point, you'll likely need to do "set tdesc filename external/valgrind/coregrind/m_gdbserver/arm-with-vfpv3.xml", because those xml files aren't getting copied to the device.  (They can also be pushed there manually to avoid the need to do the tdesc set command.)

mwu, any idea how to do that?  Need external/valgrind/coregrind/m_gdbserver/arm*.xml to get copied to /system/lib/valgrind -- I wasn't sure what rules I could use for Android.mk, or if they can just be done in another way.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: [part2] use gzip to send stuff, and pass flags better

Followup -- this uses gzip to send over the full libxul.so instead of transferring it uncompressed.  Here's the times:

vladimir@linux64$ time gzip < libxul.so > libxul.so.gz
real	0m43.711s

vladimir@linux64$ ls -l libxul.so*
-rwxr-xr-x 1 vladimir vladimir 840353560 Feb 26 12:49 libxul.so
-rw-r--r-- 1 vladimir vladimir 340899961 Feb 26 13:36 libxul.so.gz

vladimir@linux64$ time adb push libxul.so.gz /data/valgrind-b2g
850 KB/s (340899961 bytes in 391.290s)
real	6m31.382s

vladimir@linux64$ time adb shell gzip -d /data/valgrind-b2g/libxul.so.gz
real	0m40.398s

so total of ~8 minutes.  Sending the full 840mb at 850KB/s would have taken ~15 min.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(Last step should actually be "./run-gdb.sh vgdb 1234" -- to specify the port number.  Maybe we can make some port the default.)

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8382295 [details] [diff] [review]
b2g-vgdb.patch

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

Overall I'm good with this, just had a couple of nits/questions.

run-gdb and run-valgrind both go in the b2g repo. The Android.mk would need to be added to the fxos branch of the valgrind repo. I can land all of this once we're ready.

::: run-valgrind.sh
@@ +36,3 @@
>  else
> +  echo "Did you forget nocopy? Sleeping for 10 so you can cancel"
> +  sleep 10

Most of the usage for valgrind so far had been check->fix->compile->push->check to fix leaks, which is why we defaulted to the copy. Best bet may be to actually force people to use a "copy" argument versus just defaulting behavior?

@@ +44,5 @@
>  
> +echo "Did you forget to tee output? Sleeping for 5"
> +sleep 5
> +
> +#$ADB reboot

That reboot was put in as insurance because we used to get into some /really/ weird states if the phone is used for a while then we boot b2g into valgrind without a reboot. Whether or not that's still the case, I don't know. I'll try it out and see.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8382414 [details] [diff] [review]
[part2] use gzip to send stuff, and pass flags better

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

::: run-valgrind.sh
@@ +47,5 @@
> +  time adb push $GECKO_OBJDIR/toolkit/library/libxul.so.gz $B2G_DIR/libxul.so.gz
> +
> +  echo "Decompressing on phone..."
> +  time adb shell "gzip -d $B2G_DIR/libxul.so.gz"
> +  adb shell "chmod 0755 $B2G_DIR/libxul.so"

Ok I'm all for this part.

@@ +58,4 @@
>  $ADB wait-for-device
>  $ADB shell stop b2g
>  
> +VALGRIND_ARGS="--soname-synonyms=somalloc=NONE"

It sounds like this might be more work than we were planning on though? Might break this out into bug 976950 and land all of the other stuff we got here?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> That reboot was put in as insurance

Whoops, didn't notice I commented that out. It's working fine for me without it, but it's definitely not necessary to nuke it.  (It complicates matters for me because I'm using a VM and have to manually reconnect the USB device on reboot.)

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Enable vgdb building in valgrind

Originally vlad's patch with an r? to me, but I'm cool with it so marking r+. Just making a new attachment to divide it out from the run scripts.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Add vgdb running capabilities to run-valgrind/gdb scripts

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v2) - gzip libxul.so before sending to device

Fixing the only r- issue I had with this in :vlad's version and r+'ing so we can get it landed.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8386315 [details] [diff] [review]
Patch 1 (v1) - Enable vgdb building in valgrind

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

Wow. I'm r-'ing myself for posting a malformed patch. This is a first.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Enable vgdb building in valgrind

Fixing patch so we don't double up the build target

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 8386318 [details] [diff] [review]
Patch 2 (v2) - Add vgdb running capabilities to run-valgrind/gdb scripts

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

Please use -U8 when creating patch diffs.

Looks reasonable to me.

::: run-valgrind.sh
@@ +56,3 @@
>  # Due to the fact that we follow forks, we can't log to a logfile. Expect the
>  # user to redirect stdout.
> +$ADB shell 'B2G_DIR="/data/valgrind-b2g" HOSTNAME="b2g" LOGNAME="b2g" COMMAND_PREFIX="/system/bin/valgrind -v --fair-sched=try '"$VALGRIND_ARGS"' --error-limit=no --smc-check=all-non-file" exec /system/bin/b2g.sh'

nit: I think that this would have been easier to parse if you flipped your quotes around.

$ADB shell "B2G_DIR='/data/valgrind-b2g' HOSTNAME='b2g' LOGNAME='b2g' COMMAND_PREFIX='/system/bin/valgrind -v --fair-sched=try $VALGRIND_ARGS --error-limit=no --smc-check=all-non-file' exec /system/bin/b2g.sh"

And, in fact, all of the single quoted strings except for COMMAND_PREFIX could be unquoted (but I often leave that type of thing quoted because it gets colorized nicely in my editor).

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Enable vgdb building in valgrind and copy xml files, add arm64 sources

Adds proper vgdb target, adds copy for xml files needed to use vgdb, add arm64 sources added since last svn update

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4, Final) - Enable vgdb building in valgrind and copy xml files, add arm64 sources

Forgot to add vgdb to the product packages

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v4, final) - Add vgdb running capabilities to run-valgrind/gdb scripts, gzip libxul.so before sending to device; r=qdot

Fixed adb finding in scripts.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Valgrind makefile updates:

https://github.com/mozilla-b2g/valgrind/commit/daa61633c32b9606f58799a3186395fd2bbb8d8c

B2G Script Updates:

https://github.com/mozilla-b2g/B2G/commit/1727144c2fbbfea29de3f4c9e7fdca8bfef3c9cd