847764 - Add race-detector annotations to NSPR

Status: NEW

(NSPR :: NSPR, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

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 1

[(not currently active) Ted Mielczarek]

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 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 3

[Timothy B. Terriberry (:derf)]

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.