Closed Bug 854517 Opened 11 years ago Closed 11 years ago

Integrate valgrind into B2G builds

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(9 files, 6 obsolete files)

2.00 KB, text/plain
Details
1016 bytes, patch
Details | Diff | Splinter Review
943 bytes, patch
Details | Diff | Splinter Review
3.51 KB, patch
khuey
: review+
Details | Diff | Splinter Review
4.59 KB, patch
jseward
: review+
Details | Diff | Splinter Review
30.34 KB, patch
jseward
: review+
Details | Diff | Splinter Review
838 bytes, patch
mwu
: review+
Details | Diff | Splinter Review
6.11 KB, patch
mwu
: review+
Details | Diff | Splinter Review
442 bytes, patch
Details | Diff | Splinter Review
Take ben's instructions, and make the build system do it. This includes getting the kernel with expanded memory, integrating valgrind building into android, putting it all on the phone, etc...
Ok. Built off the head of a valgrind git I found on github that I'll probably fork over to mozilla-b2g. Using ndk and platform directories from inside our build, got a version of valgrind onto the phone that seems to run fine, though I have yet to actually try to check b2g with it, just tried a couple of different things on the phone.

I figure we'll maintain a fork of valgrind that we'll just rebase our patches on top of for the time being, and that will build assuming engineering build flags are on?
Found that android is actually running their own android fork of valgrind now:

https://android.googlesource.com/platform/external/valgrind/

After talking to jseward some, it sounds like our best bet is to make our own fork following his changes, so we aren't waiting for whatever may or may not come into android's valgrind repo. He's looking at integrating some of the patches from both bent and the android repo over the next few weeks, and until then we can just run a branch with those patches sitting on top while we wait for integration.

Our own tracking repo will be at

http://www.github.com/mozilla-b2g/valgrind
It looks like the github.com branch is missing libvex.  I pulled the svn repo instead and that built fine.
It's actually a completely different repo. there's mozilla-b2g/valgrind and mozilla-b2g/vex.
The instructions in the android readme for valgrind assume you're using an NDK. As FxOS is a special snowflake of a build tree, we scatter the needed directories all over the place. Drop this script in your valgrind checkout to set it up for hand building in B2G. This will all be automated by the time this lands, this is just a stopgap.
Comment on attachment 729283 [details]
bent's instructions for phone valgrind

With |export DISABLE_JEMALLOC=1| in my .userconfig, I get:

configure: error: --enable-replace-malloc requires --enable-jemalloc

Removing it allows the build to proceed.
Comment on attachment 729283 [details]
bent's instructions for phone valgrind

Another problem I'm running into: it looks like |adb push $GECKO_OBJDIR/toolkit/library/libxul.so /sdcard| never finishes.  I sniffed the adb connection with Wireshark and see ~150MB get transferred (this is to the emulator, so it's pretty quick) and then the connection stalls (Wireshark says the emulator-side's receive window is closed).  I've left adb-push running for hours and no progress.

Any sense of how long this should take?
Comment on attachment 729283 [details]
bent's instructions for phone valgrind

I've been examining this with various sizes of files, and have found that I can reliably send a 50MiB file, but not a 100MiB file.

Also, in |adb push $GECKO_OBJDIR/toolkit/library/libxul.so /sdcard|, the destination needs to be /sdcard/, or you'll get (e.g.) "failed to copy '50MB.file' to '/sdcard': Is a directory".
I just realized the emulator only creates an 512MiB 'sdcard' by default, which of course isn't big enough to hold the ~750MiB libxul.so; I modified run-emulator.sh to create an 8GiB sdcard image instead, but still only see ~100-150MiB of libxul.so get transferred before the TCP receive window crashes.
It looks like adb-push to the emulator is randomly broken.  A better approach may be to drop the desired libxul.so into 'out/target/product/generic/data' and then run |$B2G/bld.sh userdataimage| to include the file in the /data partition.

The /data partition will grow from the default of 512MiB to accommodate the larger file, but without any free space, which will prevent b2g from starting.  To make more space, you need to increase --partition-size in $B2G/run-emulator.sh from 512 to (e.g.) 1024.  (This will also increase the size of the /system partition as well.)
Comment on attachment 729283 [details]
bent's instructions for phone valgrind

When modifying /system/bin/b2g.sh, |vdc volume mount sdcard| must appear _before_ any LD_PRELOAD statements, or the command will segfault and Valgrind won't be able to write to /sdcard.
Need to add valgrind includes to configure.in for base and js
Adds valgrind to image if environment variable is set
Attachment #748256 - Flags: review?(mwu)
Adding valgrind to b2g.mk, adding build flags for gecko if B2G_VALGRIND environment variable is defined
Attachment #748257 - Flags: review?(mwu)
Depends on: 871739
All I'm doing in this patch is adding a pregenerated libvex_guest_offsets.h file for inclusion. However, I want to make sure that's an OK thing to do via Julian. Google's android forks of valgrind do this, since there's no configure to be run per platform.
Attachment #749008 - Flags: review?(jseward)
Valgrind patches for b2g tree integration. Really just involves a symlink to vex (since I'm not sure repo deals with repos-in-repos well), the patches julian already made for bent, the android.mk (modified from the android project's tree), and a pregenerated config.h. Once again, mainly looking for verification that nothing is wildly wrong, it does seem to build and run ok using these flags.
Attachment #749009 - Attachment is obsolete: true
Attachment #749012 - Flags: review?(jseward)
Comment on attachment 749008 [details] [diff] [review]
Patch 5 (v1) - VEX changes for B2G valgrind branch

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

That seems OK.  libvex_guest_offsets.h contains offsets of some
fields of the struct VexGuestARMState defined in libvex_guest_arm.h,
so that V's JIT can generate code that references that struct.

Hence libvex_guest_offsets.h will need to be updated when the
struct changes, but it basically hardly ever changes (< once per
year).  In any case I think that V will fail in a very obvious
way if the two get out of sync.
Attachment #749008 - Flags: review?(jseward) → review+
Comment on attachment 749012 [details] [diff] [review]
Patch 6 (v1) - Valgrind patches for B2G valgrind branch

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

Seems fine.
Attachment #749012 - Flags: review?(jseward) → review+
Comment on attachment 748256 [details] [diff] [review]
Patch 2 (v1) - Changes to platform-build for valgrind

We don't use this copy of b2g.mk anymore (unless you want to uplift to v1-train)
Attachment #748256 - Flags: review?(mwu)
Comment on attachment 748257 [details] [diff] [review]
Patch 3 (v1) - Valgrind changes for gonk-misc repo

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

::: b2g.mk
@@ +15,5 @@
>  	sources.xml \
>  	$(NULL)
>  
> +ifneq ($(B2G_VALGRIND),)
> +PRODUCT_PACKAGES += \

Can we get this into a list maintained in the valgrind fork? Then we can just include external/valgrind/valgrind.mk or whatever here.
Attachment #748257 - Flags: review?(mwu)
Changed list of needed packages to be included from repo instead of having verbose listing in file.
Attachment #750091 - Flags: review?(mwu)
Attachment #748257 - Attachment is obsolete: true
Comment on attachment 748256 [details] [diff] [review]
Patch 2 (v1) - Changes to platform-build for valgrind

Obsoleting old build patch for now. Will open new bug for uplifting valgrind to v1-train if needed.
Attachment #748256 - Attachment is obsolete: true
Backed out a bit too far on the last patch, was missing default-gecko-config changes.
Attachment #750091 - Attachment is obsolete: true
Attachment #750091 - Flags: review?(mwu)
Attachment #750095 - Flags: review?(mwu)
Attachment #750095 - Flags: review?(mwu) → review+
Attachment #748250 - Flags: review?(khuey)
Just planning on tracking the fxos branch heads for the moment.
Attachment #751779 - Flags: review?(mwu)
Attachment #751779 - Attachment description: Patch 4 (v1) - B2G Manifests Update → Patch 4 (v2) - B2G Manifests Update
Attachment #748950 - Attachment is obsolete: true
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #28)
> Created attachment 751779 [details] [diff] [review]
> Patch 4 (v2) - B2G Manifests Update
> 
> Just planning on tracking the fxos branch heads for the moment.

It looks like this is against the v1-train manifests. We probably want the master manifests, unless you're planning to uplift the gonk-misc changes.
Patched against b2g-manifest repo master branch instead of v1-train
Attachment #751779 - Attachment is obsolete: true
Attachment #751779 - Flags: review?(mwu)
Attachment #751799 - Flags: review?(mwu)
Comment on attachment 751799 [details] [diff] [review]
Patch 4 (v3) - B2G Manifests Update

Looks ok to me.

Just be sure to watch TBPL/#developers in case anything goes horribly wrong when this lands.
Attachment #751799 - Flags: review?(mwu) → review+
This is actually a component of Patch 3, but it goes in a different repo.
Patch 4 revert /again/ due to symlink not working. Need to change include path to deal with the ../../VEX hardlink in some parts of the gdbserver code in valgrind.
Comment on attachment 749012 [details] [diff] [review]
Patch 6 (v1) - Valgrind patches for B2G valgrind branch

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

::: Android.mk
@@ +14,5 @@
> +
> +LOCAL_PATH:= $(call my-dir)
> +
> +ifeq ($(TARGET_DEVICE), generic)
> +  ANDROID_HARDWARE := ANDROID_HARDWARE_emulator

It can also be 'generic_x86' for emulator-x86 and 'generic_mips' for emualtor-mips.

@@ +59,5 @@
> +
> +# Build libvex-($TARGET_ARCH)-linux.a
> +include $(CLEAR_VARS)
> +
> +LOCAL_MODULE := libvex-$(TARGET_ARCH)-linux

You're building target specific static library.  Do we really have to embed $(TARGET_ARCH) in module name?

@@ +417,5 @@
> +	callgrind/jumps.c \
> +	callgrind/main.c \
> +	callgrind/sim.c \
> +	callgrind/threads.c \
> +	cachegrind/cg-arch.c 

trailing ws.
Comment on attachment 752859 [details] [diff] [review]
Patch 7 (v1) - Package Include File for Valgrind Repo

>PRODUCT_PACKAGES += \
>	libvex-arm-linux \

Since you have |libvex-$(TARGET_ARCH)-linux| in external/valgrind/Android.mk, you should at least follow here to prevent build errors in other target arch.
gonk-misc commit for patch 3:

https://github.com/mozilla-b2g/gonk-misc/commit/5240bf92be218c3a9e785e389903795ecb19893b

All patches are landed, emulator-x86 followup happening in bug 875517.

SHERIFF: Bug can be closed when Patch 1 (https://hg.mozilla.org/projects/birch/rev/e38cc9c56e9a) goes birch -> m-c
You need to log in before you can comment on or make changes to this bug.