424040 - Add valgrind integration hooks to jemalloc

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Andrew Schultz]

Attachment: errors from valgrind

With a build compiled on FC6 (as well as the nightlies from tinderbox), valgrind complains a lot about using uninitialized memory in multiple places.  With a build on Fedora 8, I see no errors.  Both boxes have gcc 4.1.2, although the compilers have different additional patches.

I've attached a log with some of the errors.

Additionally, valgrind was complaining about
http://mxr.mozilla.org/seamonkey/source/js/src/jsgc.c#1729
I added a line to print localMallocBytes and the error went away.  I removed the print and the error didn't come back.

valgrind also complained about jsinterp.c:121 (this is one of the errors in the attachment).  I added a line to print out cache->disabled and valgrind still complained, but it consistently printed as 0 (which doesn't seem uninitialized).

Comment 1

[Andrew Schultz]

I'm seeing this with a build from 2008-03-15-09 but not from 2008-03-14-08.

Comment 2

[Brendan Eich [:brendan]]

64-bit or 32-bit architecture?

/be

Comment 3

[Andrew Schultz]

32-bit

Comment 4

[Robert Sayre]

I can reproduce this on 32-bit and 64-bit.

Comment 5

[Robert Sayre]

This bug is observable on Ubuntu 7.10 64-bit and 32-bit.

Comment 6

[Damon Sicore (:damons)]

Both sayre and brendan agree this should block beta pending investigation.  

+'ing, P1, TM=beta5

Comment 7

[Stuart Parmenter]

Jason: this sounds like a bug calloc'ing large-sized blocks.

Comment 8

[Robert Sayre]

These errors are not present in a build using --disable-jemalloc.

Comment 9

[Brendan Eich [:brendan]]

What are the right Component and QA Contact field settings for this bug?

/be

Comment 10

[Jason Evans]

valgrind only recognizes memory allocations from functions that embed the VALGRIND_MALLOCLIKE_BLOCK() macro from valgrind/valgrind.h.  jemalloc does not currently embed the necessary macro calls for integration with valgrind.

My understanding is that ordinarily, valgrind replaces the default malloc implementation by replacing the appropriate symbols after libc is loaded.  However, the jemalloc symbols override those supplied by valgrind, thus preventing valgrind from using its own allocator.

In the absence of memory corruption or crashes that appear memory related, I do not think this is a bug.

Comment 11

[Brendan Eich [:brendan]]

Is there a bug to get jemalloc using the right VALGRIND_* macrology? That would be the bug to fix. If it's not on file, feel free to reopen and morph this one, since we really do want to treat the symptom here, not just pin this bug to a hypothesis about cause that was faulty. Thanks,

/be

Comment 12

[Jason Evans]

No, there isn't currently a bug to track adding valgrind support in jemalloc, so I'm morphing this one to track the feature addition, as you suggest.

Comment 13

[Brendan Eich [:brendan]]

Not sure we can stand the false positives in valgrind output for 1.9/fx3 -- valgrind is pretty important for pre-release testing. Any chance of a fast fix, at least in our tree?

/be

Comment 14

[Jason Evans]

I'm working on it this morning, so I should have a patch soon.

Comment 15

[Jason Evans]

Attachment: Add --with-valgrind option

The attached patch adds the --with-valgrind configure option.  If that option is enabled, and valgrind/valgrind.h is present, jemalloc compiles in the necessary valgrind macro glue to support running Firefox under valgrind.

This code should not be left enabled for production builds, because it has to disable in-place realloc().  Valgrind does not provide a way to modify the size of an existing allocation, so we have to malloc/memcpy/free instead.

I seem to have tripped on a valgrind bug while developing this patch.  An apparent workaround is included in the patch, but I do not yet know if the workaround is sufficient to always avoid problems.

The patch also removes the MALLOC_LAZY_FREE code (which was not enabled anyway).  That code was removed from the FreeBSD jemalloc (benchmarks showed it to not be a consistent improvement), and it would have required extra work to make lazy deallocation work with valgrind.

Note that 'configure' is left out of the patch, so run something like 'make -f client.mk RUN_AUTOCONF_LOCALLY=yes AUTOCONF=autoconf2.13 configure' to generate it locally before adjusting your .mozconfig and rebuilding.

I do not know how important this feature is to others, or whether it should be part of the next beta/release.  If you are in the know, please feel free to set bug status accordingly.

Comment 16

[Jason Evans]

Attachment: Add --with-valgrind option

The supposed valgrind bug turned out to be a bug in my patch.  In one place I called VALGRIND_FREELIKE_BLOCK(ptr, size), but the second argument is redzone size, and should have been 0.  This mistake caused a neighboring object deallocation to mark part of a remaining object as inaccessible, since the bogus redzone overlapped.

Comment 17

[Reed Loden [:reed]]

Why not --with-jemalloc-valgrind instead of --with-valgrind, which sounds like you're enabling some valgrind thing for all of Gecko when it's just the jemalloc valgrind hooks?

Comment 18

[Benjamin Smedberg]

I hope that we will be able to add other valgrind things to mozilla.

Comment 19

[Reed Loden [:reed]]

Comment on attachment 311149 [details] [diff] [review]
Add --with-valgrind option

(In reply to comment #18)
> I hope that we will be able to add other valgrind things to mozilla.

Ok, then we shouldn't be jemalloc specific in the description of the build option:

>+  AC_ARG_WITH([valgrind],
>+    [  --with-valgrind         Enable valgrind integration hooks in jemalloc],
>+    [enable_valgrind="yes"], [enable_valgrind="no"])
>+  AC_CHECK_HEADER([valgrind/valgrind.h], [], [enable_valgrind="no"])
>+  if test "x$enable_valgrind" = "xyes" ; then
>+    AC_DEFINE(MOZ_MEMORY_VALGRIND)
>+  fi

So, maybe "Enable valgrind integration hooks" instead and MOZ_VALGRIND or something as the define since MOZ_MEMORY_* is all jemalloc stuff?

Comment 20

[Jesse Ruderman]

The patch in bug 348798 for arenas uses VALGRIND_HINTS.

Comment 21

[Stuart Parmenter]

Attachment: use MOZ_VALGRIND instead of MOZ_MEMORY_VALGRIND

Comment 22

[Nicholas Nethercote [inactive]]

Comments #17, #18, #19 were partly addressed, but --with-valgrind still only works if --enable-jemalloc is set.  The patch at bug #475876 disentangles this dependency, making the two options independent.