884673 - Add valgrind option to b2g.sh for running b2g under valgrind

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

If the B2G_VALGRIND environment variable is set, b2g.sh should run b2g under valgrind. If there's no valgrind built onto the system, fail with an error message saying how to build valgrind into the image. Provide a run-b2g-valgrind.sh script in the b2g root to stop the current b2g process and run a new one under valgrind.

Comment 1

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

Attachment: Patch 1 (v1) - WIP: b2g.sh changes for running valgrind

Comment 2

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

Attachment: Patch 2 (v1) - WIP: run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Comment 3

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

Attachment: Patch 1 (v2) - WIP: b2g.sh changes for running valgrind

Uploaded wrong file, and was missing libxul.so backup file

Comment 4

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

Comment on attachment 766187 [details] [diff] [review]
Patch 1 (v2) - WIP: b2g.sh changes for running valgrind

I feel kind of weird putting this in our main startup script. Might it be worth thinking about making a b2g-debug.sh or something that only runs for debug/eng builds?

Comment 5

[Michael Wu [:mwu]]

I think we want a command line arg on b2g.sh that let's us specify something to prefix to the b2g call. Something like:

exec $(COMMAND_PREFIX) /system/b2g/b2g

So this would make it easier for us to run b2g under valgrind, gdb, or anything else.

Also, using start b2g to run b2g.sh is a bit awkward. Can we just run b2g.sh directly out of the valgrind script?

Comment 6

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

(In reply to Michael Wu [:mwu] from comment #5)
> I think we want a command line arg on b2g.sh that let's us specify something
> to prefix to the b2g call. Something like:
> 
> exec $(COMMAND_PREFIX) /system/b2g/b2g
> 
> So this would make it easier for us to run b2g under valgrind, gdb, or
> anything else.

Ah, ok, think I misunderstood what you meant by command line arg the first time around.

> Also, using start b2g to run b2g.sh is a bit awkward. Can we just run b2g.sh
> directly out of the valgrind script?

If we run b2g.sh via adb shell, it won't background correctly, will it? That's why I was using start/stop, because it deals with the daemonization of the process?

Comment 7

[Michael Wu [:mwu]]

Daemonization is good for the cases where you'd want to disconnect your usb cable while still letting b2g run. Is that what we want to do with valgrind?

Everything should work normally afaik when running b2g.sh directly from a root shell.  We used to require "start b2g" to set things up properly but b2g.sh has all that logic integrated.

Comment 8

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

(In reply to Michael Wu [:mwu] from comment #7)
> Daemonization is good for the cases where you'd want to disconnect your usb
> cable while still letting b2g run. Is that what we want to do with valgrind?

It's more the problem of losing connection/shell context with valgrind running, since I'm expecting some of our marionette test runs can take hours? I may be pre-optimizing though. :)

Comment 9

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

Attachment: Patch 1 (v2) - b2g.sh changes for running b2g as part of another process (valgrind, gdb, etc)

Changed to using COMMAND_PREFIX.

Comment 10

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

Attachment: Patch 2 (v2) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Comment 11

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

So the two patches seem to work now and are far more minimal for changes to b2g.sh than the WIPs. However, now I'm running into a problem of how to deal with the libxul.so change. If we run valgrind then reboot, sdcard won't be mounted by the time b2g tries to come up, meaning we get stuck in a boot loop because the old libxul.so is now named libxul.so.bak. Do we still want to add a check in b2g.sh for that, or do you have a cleaner solution?

Comment 12

[Michael Wu [:mwu]]

Maybe add something to the script that unmounts the sdcard, adds a symlink back to libxul.so.bak in /sdcard/libxul.so, and then remounts the sdcard?

Comment 13

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

Comment on attachment 770333 [details] [diff] [review]
Patch 2 (v2) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Cancelling review to add sdcard libxul.so.bak hack.

Comment 14

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

Attachment: Patch 2 (v3) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Added reflashing warning message.

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 772270 [details] [diff] [review]
Patch 2 (v3) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

>#!/bin/bash
>#set -xv
>
>SCRIPT_NAME=$(basename $0)
>. load-config.sh
>ADB=adb
>
>HAS_VALGRIND=$($ADB shell \(test -e /system/bin/valgrind\)\; echo \$\? | tr '\r' ' ')
># Make sure valgrind is actually on system
>if [ "$HAS_VALGRIND" -ne 0 ]; then
>    echo "Platform does not have valgrind executable, did you build with B2G_VALGRIND=1 in your .userconfig?"
>    exit 1
>fi
>
># See whether system has enough RAM to run valgrind
>MEMTOTAL=$($ADB shell cat /proc/meminfo | grep MemTotal | sed "s/MemTotal:\s\{1,\}\(.*\)\s\{1,\}kB/\1/" | tr '\r' ' ')
>echo "Total System Memory (kb): $MEMTOTAL"
># Make sure valgrind is actually on system
>if [ "$MEMTOTAL" -le 400000 ]; then
>    echo "Platform most likely does not have enough free memory to run valgrind. Do you have the proper kernel loaded?"

Let's pretend the unagi does not exist and remove that last sentence.

>    exit 1
>fi
>
>echo "*** PHONE WILL NEED TO BE REFLASHED AFTER RUNNING VALGRIND ***"
>
># Load libxul
>if [ "$1" = "debuginfo" ]; then
>  echo "Recompiling libxul.so with debug info (this can take a few minutes)"
>  $ADB remount
>  cp $GECKO_OBJDIR/toolkit/library/libxul.so /tmp
>  ./prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-objcopy -R .debug_info /tmp/libxul.so

Do we need to use this particular copy of objcopy? Also, does strip work?

I think we should just stick the stripped libxul.so somewhere in the objdir rather than /tmp.

>  echo "Pushing debug libxul to phone (this takes about a minute)"
>  time adb push /tmp/libxul.so /sdcard/libxul.so
>  adb shell mv /system/b2g/libxul.so /system/b2g/libxul.so.bak
>  adb shell ln -s /sdcard/libxul.so /system/b2g/libxul.so
>elif [ "$1" = "nocopy" ]; then
>  echo "Skipping libxul.so copy step and just running valgrind..."

This is pretty terrible. Something like https://github.com/mozilla-b2g/adb-push would help us automatically avoid pushing when not necessary. We can address that later though.

>else
>  echo "Pushing debug libxul to phone (this can take upwards of 5 minutes)"
>  $ADB remount
>  time adb push $GECKO_OBJDIR/toolkit/library/libxul.so /sdcard/libxul.so
>  adb shell mv /system/b2g/libxul.so /system/b2g/libxul.so.bak
>  adb shell ln -s /sdcard/libxul.so /system/b2g/libxul.so
>fi
>
>$ADB shell vdc volume mount sdcard
>$ADB shell mount -o remount,exec /dev/block/vold/179:33 /sdcard
>

Do we need libxul.so to be executable? I don't remember the dynamic linker caring.

>VALGRIND_LOG_EXISTS=$($ADB shell \(test -f /sdcard/valgrind.log\)\; echo \$\? | tr '\r' ' ')
>
>if [ "$VALGRIND_LOG_EXISTS" -eq 0 ]; then
>  $ADB shell mv /sdcard/valgrind.log /sdcard/valgrind.log.old
>fi

Can we unconditionally move the log?

># TODO: Had to turn off fair scheduling?
>$ADB shell 'COMMAND_PREFIX="/system/bin/valgrind -v --fair-sched=yes --error-limit=no --smc-check=all-non-file --log-file=/sdcard/valgrind.log" exec /system/bin/b2g.sh'
>

Comment 16

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

(In reply to Michael Wu [:mwu] from comment #15)
> This is pretty terrible. Something like
> https://github.com/mozilla-b2g/adb-push would help us automatically avoid
> pushing when not necessary. We can address that later though.

I suppose I could try a file time creation comparison or something? I'm honestly not real happy about it either, it's gross.

> Do we need libxul.so to be executable? I don't remember the dynamic linker
> caring.

Yup. Otherwise we get

XPCOMGlueLoad error for file /system/b2g/libxul.so:
Cannot load library: load_segments[915]: 534 failed to map segment from 'libxul.so' @ 0x05346000 (0x0238b660). p_vaddr=0x00000000 p_offset=0x00000000
Couldn't load XPCOM.

Comment 17

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

Stopped trying to run valgrind on unagi and the like, since 512mb doesn't seem to be enough for running m-c under valgrind. Concentrating on the nexus 4 and jb now.

Have valgrind up and running under jb, but it requires some changes. Pushing libxul.so to /sdcard works until we try to spawn a child process, then dynamic linking fails and the OS never comes up. Pushing /all/ of b2g to /sdcard is a no go because we can't remount it as exectuable due to jb security features. The current solution is to just make a new /data/b2g that's a copy of /system/b2g, push the debug libxul.so there, and have b2g.sh point to there instead of /system/b2g, using environment variables. This seems to work, and also leaves normal b2g intact.

Comment 18

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

Attachment: Patch 1 (v3) - b2g.sh changes for running b2g as part of another process (valgrind, gdb, etc)

Ok, new tactic. Throwing libxul.so in /sdcard and trying to symlink doesn't work on 4.3, due to all sorts of reasons. However, on the n4 (which I'm going to say is the only phone we support for valgrind for the foreseeable future), we've got 5gb paritioned to /data, more than enough to hold a completely seperate version of b2g with the debug libxul.so. However, we need to be able to specify to run from there. This adds a $B2G_DIR variable for where B2G should be on the phone, defaulting to /system/b2g.

Comment 19

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

Attachment: Patch 2 (v4) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Assuming we're going to rerun valgrind, the system needs to be cleaned out of old valgrind runs, recopied, and rebooted. Running valgrind multiple times on the same boot causes weirdness. Script now takes care of all this.

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 786651 [details] [diff] [review]
Patch 1 (v3) - b2g.sh changes for running b2g as part of another process (valgrind, gdb, etc)

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

Looks good if we can avoid additional nesting, but I'll want to see a new patch since the addition changes added by nesting makes things a bit hard to read.

::: b2g.sh
@@ +5,5 @@
>  chmod 1777 $TMPDIR
>  ulimit -n 8192
>  
> +if [ -z "$B2G_DIR" ]; then
> +    B2G_DIR="/system/b2g"

Don't think we need to nest the if statements.

Comment 21

[Michael Wu [:mwu]]

Comment on attachment 786652 [details]
Patch 2 (v4) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

>#!/bin/bash
>#set -xv
>
>SCRIPT_NAME=$(basename $0)
>. load-config.sh
>ADB=adb

ADB=${ADB:-adb}

is how flash.sh does it. It also has heuristics for finding ADB if one isn't installed in $PATH that you can steal.

>B2G_DIR="/data/valgrind-b2g"
>
>HAS_VALGRIND=$($ADB shell \(test -e /system/bin/valgrind\)\; echo \$\? | tr '\r' ' ')

HAS_VALGRIND=$($ADB shell 'test -e /system/bin/valgrind ; echo -n $?')

is a bit simpler and seems to work here.

># Make sure valgrind is actually on system
>if [ "$HAS_VALGRIND" -ne 0 ]; then
>    echo "Platform does not have valgrind executable, did you build with B2G_VALGRIND=1 in your .userconfig?"
>    exit 1
>fi
>
># See whether system has enough RAM to run valgrind
>MEMTOTAL=$($ADB shell cat /proc/meminfo | grep MemTotal | sed "s/MemTotal:\s\{1,\}\(.*\)\s\{1,\}kB/\1/" | tr '\r' ' ')
>echo "Total System Memory (kb): $MEMTOTAL"
># Make sure valgrind is actually on system
>if [ "$MEMTOTAL" -le 400000 ]; then
>    echo "Platform most likely does not have enough free memory to run valgrind."

We should specify the number we used for this heuristic so people know what sort of device they should try.

>    exit 1
>fi
>
>#Load libxul

Space after #

>if [ "$1" = "debuginfo" ]; then
>  echo "Recompiling libxul.so with debug info (this can take a few minutes)"
>  $ADB remount
>  $ADB shell rm -rf $B2G_DIR
>  $ADB shell mkdir -p $B2G_DIR

We should be able to skip this and let cp make the directory as long as we get rid of that wildcard in the path.

>  $ADB shell cp -r /system/b2g/* $B2G_DIR
>  cp $GECKO_OBJDIR/toolkit/library/libxul.so $GECKO_OBJDIR/toolkit/library/libxul.debuginfo.so
>  ./prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-strip -R .debug_info $GECKO_OBJDIR/toolkit/library/libxul.debuginfo.so

We actually use the 4.7 toolchain these days. Also, the prebuilt directory doesn't exist on gonk-jb.

You might be able to include .var.profile and use TARGET_TOOLS_PREFIX.

>  echo "Pushing debug libxul to phone (this takes about a minute)"
>  time adb push $GECKO_OBJDIR/toolkit/library/libxul.debuginfo.so $B2G_DIR/libxul.so
>elif [ "$1" = "nocopy" ]; then
>  echo "Skipping libxul.so copy step and just running valgrind..."
>else
>  echo "Pushing debug libxul to phone (this can take upwards of 5 minutes)"
>  $ADB remount
>  $ADB shell rm -rf $B2G_DIR
>  $ADB shell mkdir -p $B2G_DIR

Ditto about not needing to mkdir.

>  $ADB shell cp -r /system/b2g/* $B2G_DIR
>  time adb push $GECKO_OBJDIR/toolkit/library/libxul.so $B2G_DIR/libxul.so
>fi
>
>$ADB reboot
>$ADB wait-for-device
>$ADB shell stop b2g
>
># 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" COMMAND_PREFIX="/system/bin/valgrind -v --fair-sched=try --trace-children=yes --error-limit=no --smc-check=all-non-file" exec /system/bin/b2g.sh'
>

Comment 22

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

Attachment: Patch 1 (v4) - b2g.sh changes for running b2g as part of another process (valgrind, gdb, etc)

Comment 23

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

Attachment: Patch 2 (v5) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Comment 24

[Michael Wu [:mwu]]

Comment on attachment 795768 [details] [review]
Patch 1 (v4) - b2g.sh changes for running b2g as part of another process (valgrind, gdb, etc)

Looks good to me. Might want to revise it to add a few quotes (as jld mentioned) in case we want to use B2G_DIRs that have spaces in them in the future.

Comment 25

[Michael Wu [:mwu]]

Comment on attachment 795797 [details] [review]
Patch 2 (v5) - run-valgrind.sh script for doing sanity checks, setting up system, running valgrind

Looks good.

Comment 26

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

https://github.com/mozilla-b2g/gonk-misc/commit/a3174c6bc5fd6b9552509c779bef8d5cd1f53c92
https://github.com/mozilla-b2g/B2G/commit/6ee0d5d20cf9a9dfe1392de7dd9c3b29e772709c