848303 - Add race-detector annotations header to MFBT

Status: NEW

(Core :: General, defect)

Description

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

Attachment: Patch, v1

Spun off from bug 551155. I'd like to get this RaceChecking.h patch into mfbt so that we can start annotating all the things we need for helgrind to be useful.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 721661 [details] [diff] [review]
Patch, v1

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

In the absence of quick examples that demonstrate how/why these would be used, it's hard to really conceptualize them well from just the header, and to know when to use them.  With the header as it is, I expect these would be used only when some random person who happened to know enough about them actually asked for their use.  But if these are to be any good, they need to be something not just the informed few can find, and use.  So some examples here are essential.

These macros all seem incredibly low-level, probably because they are.  :-)  Hopefully we can add all these to mozilla::Atomic, in bug 732043, whenever that happens, so that people don't have to know about them.  Or do I not understand how these macros would work in practice?

::: configure.in
@@ +1662,5 @@
>  [  --enable-valgrind       Enable Valgrind integration hooks (default=no)],
>      MOZ_VALGRIND=1,
>      MOZ_VALGRIND= )
>  if test -n "$MOZ_VALGRIND"; then
> +    MOZ_CHECK_HEADERS(valgrind/valgrind.h valgrind/memcheck.h valgrind/helgrind.h, [],

Does this change, or something like it, need to happen to js/src/configure.in as well?  I'd bet so.

::: mfbt/RaceChecking.h
@@ +15,5 @@
> + *     you can safely call this multiple times provided that
> + *     MOZ_RACECHECK_ENABLE_ERROR_REPORTING is called the same number of times
> + *     to re-enable reporting.
> + *
> + * - MOZ_RACECHECK_ENABLE_ERROR_REPORTING()

I think we should make these into a single RAII class, so that disabling/enabling are always matched.  Anyone who doesn't want disabling/enabling in quite so much a scope-ish manner can use mozilla::Maybe to disable at some odder point while still having it reenabled correctly later.

@@ +42,5 @@
> + *     MOZ_RACECHECK_HAPPENS_AFTER on it has no effect on the calling thread. An
> + *     implementation may optionally release resources it has associated with
> + *     'obj' when MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL(obj) happens. Users
> + *     are recommended to use MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL to
> + *     indicate when a synchronisation object is no longer needed, so as to

synchronization, of course :-P

@@ +52,5 @@
> + * - MOZ_RACECHECK_ENABLE_CHECKING(addr, size)
> + *     Re-enable error checking, as per comments on
> + *     MOZ_RACECHECK_DISABLE_CHECKING.
> + *
> + * With no race checker available, all macros expand to the empty statement.

I might say no-ops just because "empty statement" suggests ";" or even just "" (with the ; supplied by the user).

@@ +64,5 @@
> +#include "valgrind/helgrind.h"
> +#include "valgrind/memcheck.h"
> +
> +#define MOZ_RACECHECK_DISABLE_ERROR_REPORTING \
> +  VALGRIND_DISABLE_ERROR_REPORTING

Slightly prefer () on both of these, and all the rest, to make doubly clear (past the comments above) to the reader that they're macro-methods.

@@ +87,5 @@
> +
> +#else /* MOZ_VALGRIND */
> +
> +#define MOZ_RACECHECK_DISABLE_ERROR_REPORTING do {} while (0)
> +#define MOZ_RACECHECK_ENABLE_ERROR_REPORTING do {} while (0)

() here too.  I'm actually surprised you can call a macro not defined as a method -- I'd have expected it to expand to do {} while (0)(), which obviously wouldn't compile.

@@ +96,5 @@
> +#define MOZ_RACECHECK_ENABLE_CHECKING(addr, size) do {} while (0)
> +
> +#endif /* MOZ_VALGRIND */
> +
> +#endif  /* mozilla_MemoryChecking_h_ */

RaceChecking copypasta awesomesauce

Comment 2

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

Attachment: Patch, v1.1

Ok, how's this look?

Comment 3

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Does this change, or something like it, need to happen to
> js/src/configure.in as well?  I'd bet so.

Yep, it's in my local patch, somehow I cut it out of the patch I submitted.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 724525 [details] [diff] [review]
Patch, v1.1

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

I confess to still being mostly baffled by this, but the comments help a little bit, at least.  Also consulting valgrind docs a little (nice example ;-) ).  I suppose the people who want this are going to find it in somewhat short order and/or know what they want, hopefully.

This is kind of rubberstampy, so let's have njn (or do I want jseward?) or someone who knows these underlying macros take a look as well.

::: js/src/configure.in
@@ +3608,5 @@
>  [  --enable-valgrind       Enable Valgrind integration hooks (default=no)],
>      MOZ_VALGRIND=1,
>      MOZ_VALGRIND= )
>  if test -n "$MOZ_VALGRIND"; then
> +    MOZ_CHECK_HEADERS(valgrind/valgrind.h valgrind/memcheck.h valgrind/helgrind.h, [],

You meant to drop the [] around the header names, right, here and the other?

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 724525 [details] [diff] [review]
Patch, v1.1

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

jseward knows a lot more about Helgrind than I do.

Comment 6

[Julian Seward [:jseward]]

Comment on attachment 724525 [details] [diff] [review]
Patch, v1.1


r+ if you rename mozilla::DisableRaceChecking.  The rest of it is just
an attempt at clarification; feel free to use/ignore as you like.

>+ * This class is provided:
>+ *
>+ * - mozilla::DisableRaceChecking
>+ *     Disables error reporting for the current thread when constructed. Error
>+ *     reporting is reenabled on destruction. This class behaves in a stack like
>+ *     way, so you can safely create multiple nested instances.

Good that this is RAII.  I think the name is a bit misleading though.
This doesn't disable race checking per se.  What it does (at least how
you have implemented it, on valgrind) is to turn off error reporting,
so that any errors (not just races) that any Valgrind tool (not just
Helgrind) detects, are not shown.  Hence I'd say mozilla::DisableErrorReporting
might be a better name.

>+ * The following macros are provided:
>+ *
>+ * - MOZ_RACECHECK_DISABLE_CHECKING(addr, size)
>+ *     Instruct the race detector to ignore the memory range 'addr' + 'size'.
>+ *
>+ * - MOZ_RACECHECK_ENABLE_CHECKING(addr, size)
>+ *     Re-enable error checking, as per comments on
>+ *     MOZ_RACECHECK_DISABLE_CHECKING.

... whereas the names on these are OK, since they really do pertain
only to error reporting.  You might want to add, in the comment for
MOZ_RACECHECK_DISABLE_CHECKING, that (at least for an implementation
on H) the memory will automatically be brought back into 'checkable'
mode when it is deallocated and thence reallocated.  Or at least
make some reference to what happens to memory that is deallocated
after _DISABLE_ is done on it, but before _ENABLE_ is done.

>+ * These three macros are provided to tell the race detector about the order of
>+ * inter-thread memory accesses. They are most commonly used to describe the
>+ * effects of atomic reference counting like so:

Yeah .. as Waldo says these are a bit mysterious.

>+ *   class MyClass
>+ *   {
>+ *     unsigned int mRefCount;
>+ *     void Release()
>+ *     {
>+ *       unsigned int newCount = atomic_decrement(&mRefCount);
>+ *       if (newCount == 0) {
>+ *         MOZ_RACECHECK_HAPPENS_AFTER(&mRefCount);
>+ *         MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL(&mRefCount);
>+ *         delete this;
>+ *       } else {
>+ *         MOZ_RACECHECK_HAPPENS_BEFORE(&mRefCount);
>+ *       }
>+ *     }
>+ *   };

The intention is to tell the race checkers that, once the refcount has
dropped to zero, that the calling thread 'owns' the object and so can
destruct it safely without locking.  All threads that perform a
N+1 -> N transition, for N > 0, notify the tool that "I was here".
The thread performing the 1 -> 0 transition tells the tool that 
"this has happened after (in a thread-synchronisation sense) all of
the to-non-0 transitions", and hence memory accesses made by this
thread do not conflict with the memory accesses by the other threads
before there "I was here" markers.

The 1 -> 0 thread then tells the tools that they can release any
internal resources used to track these inter-thread dependencies (via
_FORGET_ALL_) since they are no longer relevant.

>+ * - MOZ_RACECHECK_HAPPENS_BEFORE(obj)
>+ *     Insert this macro just after an access to the variable at the specified
>+ *     address has been performed. Also see comment below for
>+ *     MOZ_RACECHECK_HAPPENS_AFTER.
>+ *
>+ * - MOZ_RACECHECK_HAPPENS_AFTER(obj)
>+ *     This macro informs the race detector that the next access to the variable
>+ *     at the specified address should be considered to have happened after the
>+ *     access just before the latest MOZ_RACECHECK_HAPPENS_BEFORE annotation
>+ *     that references the same variable.

Not just "the latest" .. it is considered to have happened-after _all_
of the MOZ_RACECHECK_HAPPENS_BEFORE annotations using the same tag
(obj).

The explanation in helgrind/helgrind.h tries to formalise it a bit
more.

>+ * - MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL(obj)
>+ *     This macro instructs the race detector to forget about any
>+ *     MOZ_RACECHECK_HAPPENS_BEFORE calls on the specified object, in effect
>+ *     putting it back in its original state. Once in that state, a use of
>+ *     MOZ_RACECHECK_HAPPENS_AFTER on it has no effect on the calling thread. An
>+ *     implementation may optionally release resources it has associated with
>+ *     'obj' when MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL(obj) happens. Users
>+ *     are recommended to use MOZ_RACECHECK_HAPPENS_BEFORE_FORGET_ALL to
>+ *     indicate when a synchronization object is no longer needed, so as to
>+ *     avoid potential indefinite resource leaks.

indefinite resource leaks in the race detectors, not the app (just to
be clear :)

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Julian Seward from comment #6)
> Hence I'd say mozilla::DisableErrorReporting might be a better name.

DisableRaceReporting maybe?  "error reporting" is a bit vague and could apply to any number of kinds of errors.

Oh, and this reminds me -- use mozilla/GuardObjects.h macros to prevent someone from doing

  mozilla::DisableRaceReporting();

i.e. creating a temporary that's destroyed immediately, rather than an actual local value that lives til the scope ends.

Comment 8

[Julian Seward [:jseward]]

Attachment: Rebased version of 1.1; no other changes.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.