Part of bug 551155. I omitted changes to nsprpub/configure since they're super noisy, but I'll apply them on checkin. This is Julian's patch.

Comment on attachment 721028 [details] [diff] [review] Patch, v1 Review of attachment 721028 [details] [diff] [review]: ----------------------------------------------------------------- Superficially this looks ok, but I don't actually understand these annotations well enough to feel like I can review this sanely. Is there someone else (who isn't the patch author) that could review the content of the annotations? ::: nsprpub/configure.in @@ +3091,5 @@ > +[ --enable-valgrind Enable Valgrind integration hooks (default=no)], > + MOZ_VALGRIND=1, > + MOZ_VALGRIND= ) > +if test -n "$MOZ_VALGRIND"; then > + AC_CHECK_HEADER(valgrind/valgrind.h) nit: you don't actually use valgrind.h anywhere. ::: nsprpub/pr/include/private/pprvalgrind.h @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef pprvalgrind_h___ Missing a descriptive comment like its sibling headers have.

Comment on attachment 721028 [details] [diff] [review] Patch, v1 Tim, can you take a look at just the annotations part? Ted has already looked over the build changes.

Comment on attachment 721028 [details] [diff] [review] Patch, v1 Review of attachment 721028 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine, except for some comments and a few bits that are unnecessary. With that stuff fixed, r=me. ::: nsprpub/pr/src/pthreads/ptsynch.c @@ +103,5 @@ > PR_ASSERT(NULL != cv); > PR_ASSERT(0 != notified->cv[index].times); > if (-1 == notified->cv[index].times) > { > + /* Hide warning about cv->mx not being held here. */ cv->lock->mutex, you mean. @@ +113,5 @@ > else > { > while (notified->cv[index].times-- > 0) > { > + /* Hide warning about cv->mx not being held here. */ Ditto. @@ +291,5 @@ > PR_ASSERT(_PT_PTHREAD_MUTEX_IS_LOCKED(cvar->lock->mutex)); > > + /* Forget any previous sync points associated with cvar. */ > + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(cvar); > + ANNOTATE_HAPPENS_BEFORE(cvar); As I discussed with jseward on IRC, I don't think you actually need these annotations. Any use of the cvar should be protected by its mutex, which can provide the necessary happens-before edge. If something uses a cvar without the mutex, we want to know (though I believe we already have asserts guarding against that). @@ +355,5 @@ > #endif > PR_Free(cvar); > } > + /* Free any race-detector resources associated with CVAR. */ > + ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(cvar); Not necessary, see above. @@ +400,5 @@ > rv = pt_TimedWait(&cvar->cv, &cvar->lock->mutex, timeout); > > + if (rv == 0) { > + ANNOTATE_HAPPENS_AFTER(cvar); > + } Not necessary, see above. @@ +465,5 @@ > PR_SetError(PR_OUT_OF_MEMORY_ERROR, 0); > return NULL; > } > > + VALGRIND_HG_DISABLE_CHECKING(&mon->owner, sizeof(mon->owner)); This race has been reported as bug 844784. I'm okay with disabling the checks for now, but you should add a comment with that bug number and note that you're disabling them in the bug. ::: nsprpub/pr/src/pthreads/ptthread.c @@ +366,5 @@ > pthread_t id; > > + /* > + * thred->id is subject to a W-W race; see comments on assignments to it > + * elsewhere in this file. I don't believe the argument that this race is safe, even without resorting to the fact that this is undefined behavior in C++11 (see, e.g., <http://www.usenix.org/event/hotpar11/tech/final_files/Boehm.pdf> for examples where W-W races writing the same value can fail). I'm okay with disabling this check for now, but you should file a bug on the actual race and comment on it here.