TSan: data race xpcom/threads/nsThreads.cpp:1091 SetMainThreadObserver

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
4 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tsan])

(Reporter)

Description

4 years ago
TSan is complaining about read/write to sMainThreadObserver:

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.h#78

being racy.  I think the intention is that we always check for read/write of sMainThreadObserver being "done on the main thread", cf.

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#893

and other instances like it.  But there's nothing preventing the compiler from (if it deems it worthwhile) converting something like:

  if (side_effect_free_expr && sMainThreadObserver) {
    ...
  }

to:

  t1 = side_effect_free_expr;
  t2 = !!sMainThreadObserver;
  if (t1 && t2) {
    ...
  }

which is essentially what the compiler has done here, AFAICS from staring at the TSan assembly.
> and other instances like it.  But there's nothing preventing the compiler
> from (if it deems it worthwhile) converting something like:
> 
>   if (side_effect_free_expr && sMainThreadObserver) {
>     ...
>   }
> 
> to:
> 
>   t1 = side_effect_free_expr;
>   t2 = !!sMainThreadObserver;
>   if (t1 && t2) {
>     ...
>   }
> 
> which is essentially what the compiler has done here, AFAICS from staring at
> the TSan assembly.

I think we've seen tsan do this before, but it seems like a tsan bug to me.  That is the compiler may  perform this optimization, but it does not mean the initial program is racey.  aiui the compiler may move the rhs of a conditional up, but it still behaves as if the rhs was only executed after the lhs was tested.
(Reporter)

Comment 2

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > and other instances like it.  But there's nothing preventing the compiler
> > from (if it deems it worthwhile) converting something like:
> > 
> >   if (side_effect_free_expr && sMainThreadObserver) {
> >     ...
> >   }
> > 
> > to:
> > 
> >   t1 = side_effect_free_expr;
> >   t2 = !!sMainThreadObserver;
> >   if (t1 && t2) {
> >     ...
> >   }
> > 
> > which is essentially what the compiler has done here, AFAICS from staring at
> > the TSan assembly.
> 
> I think we've seen tsan do this before, but it seems like a tsan bug to me. 

Just to be clear: clang does this when compiling without tsan.

> That is the compiler may  perform this optimization, but it does not mean
> the initial program is racey.  aiui the compiler may move the rhs of a
> conditional up, but it still behaves as if the rhs was only executed after
> the lhs was tested.

The original program clearly wants sMainThreadObserver to only be touched on the main thread, but its method of doing so is invalid.  So we're into undefined behavior territory now, which is a undesirable place to be.

It's not hard to fix this to placate TSan, but it does need to be done.

Comment 3

4 years ago
I'm about 99% sure that short-circuiting means that per spec, the rhs of a && where the lhs is truthy must never be evaluated.  But as always, you can evaluate the rhs in actuality, so long as it's not observable that you've done so *in the language*.  My suspicion is compilers are okay to do the optimization in comment 0 because reads don't have any effect -- a spurious "race" permitted by the spec because no one observes it.

However: TSan is basically a debugger for purposes here.  So it's not subject to the not-observable restriction.  I'm not familiar with TSan's implementation enough, but I would be inclined to peg this as a TSan imprecision that should be fixed by recognizing spurious races, that are successfully guarded against by short-circuiting, as not actual races.

Comment 4

4 years ago
JFTR I would have used a different pattern:

nsIThreadObserver* mainThreadObserver =
  MAIN_THREAD == mIsMainThread ? sMainThreadObserver : nullptr;
...
if (mainThreadObserver)
  mainThreadObserver->AfterProcessNextEvent(this, mRunningEvent, *aResult);

I have no idea whether that's enough to placate TSan.
(Reporter)

Comment 5

4 years ago
It looks like we had a similar sort of bug in bug 939788, and the outcome was that the hoisting optimization was disabled when running with tsan (maybe not the best solution, but...).  I am using clang/llvm 3.3 for my tests, because that's the only revision I could get to work, and that revision doesn't include the upstream fix from bug 939788.  So I'm going to call this WORKSFORME.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.