1305005 - TSan: data races in js/src/vm/TypeInference.cpp TypeSet::TypeString(TypeSet::Type type)

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Julian Seward [:jseward]]

This is a new race that has appeared within the past two weeks,
and is now the most frequently TSan-reported JS related race
when running m-c.

TypeSet::TypeString(TypeSet::Type type) uses a global variable |which|
to cycle through a 4-entry buffer |bufs[4][40]|, selecting one of the
four entries for use for that call.  Unfortunately TypeSet::TypeString
is called from multiple threads, so TSan reports a race on |which|.

Even if we made |which| be atomic, there would still be a race on
|bufs[.]|.  Imagine 5 threads calling this function simultaneously.
Two of them are going to wind up with the same |which| value and so
they will have a write-write race on |bufs[which][.]|.

What purpose does this circular buffer mechanism serve?  At least
from a quick scan of the rest of TypeInference.cpp, the strings returned
by this function are handed on immediately to various kinds of logging
or printing functions.  I didn't check uses outside TypeInference.cpp, though.

Comment 1

[Julian Seward [:jseward]]

Attachment: TSan complaint

Comment 2

[Julian Seward [:jseward]]

Attachment: TSan complaints for the w-w race on |bufs[which][.]|

TSan also detects the write-write race on |bufs[which][.]|.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

sfink, you touched this stuff on September 13th (the most recent blame on the function :P), do you have any idea what's up with this?

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> sfink, you touched this stuff on September 13th (the most recent blame on
> the function :P), do you have any idea what's up with this?

Ugh, this looks to be some kind of cheap hack to avoid heap-allocation for a temporary string, where a couple of strings' lifespans overlap. It looks like I moved the code in bug 1301764, but it dates back to bug 637674. It appears to intended to be DEBUG-only, though it is now enabled unconditionally now. Ah, it's used for InferSpew.

These should probably be fixed to pass in stack-allocated buffers or something.

Comment 5

[Steve Fink [:sfink] [:s:]]

Oh, I see. Most of the time, these stringifying functions can get away with simply returning a const char* to a string literal. So the API is kind of built around that, and properly handling buffers is a little clunky for a debugging feature.

Comment 6

[Steve Fink [:sfink] [:s:]]

Ugh. And when taking a quick stab at it, I'm nervous about passing through stack allocated buffers everywhere, because it would bloat up the frame size of several hot type inference functions, and for all I know that might be a big deal.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Remove race on TypeString() char buffers

This is the mindless, mechanical way of fixing it. But as I mentioned, it bloats up a bunch of stack frames, I'm ambivalent as to whether it's a good idea to land this and see whether it regresses anything. It would be better to do something fancy with returning a UniqueChars or something and discarding it at the right time, but I didn't want to take the time right now to design that out.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8914528 [details] [diff] [review]
Remove race on TypeString() char buffers

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

::: js/src/vm/TypeInference.cpp
@@ -131,5 @@
>          return NonObjectTypeString(type);
>  
> -    static char bufs[4][40];
> -    static unsigned which = 0;
> -    which = (which + 1) & 3;

Wow, that's pretty bad!  I sure hope this doesn't get used more than four times in single expression.

I don't really like having to allocate stack buffers in advance though.

How about allocating and returning a UniqueChars from these?  As I understand it, the rules around temporary objects mean that the returned temporary will live long enough (until the end of the complete expression containing it) so you won't have to do anything fancy.  You may need to call .get() to get the char pointer out.

Comment 9

[Steve Fink [:sfink] [:s:]]

Aw man, I was being lazy. That is indeed what I was considering doing, but was scared off from figuring out the rules around temporary objects. But ok:

"All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation. This is true even if that evaluation ends in throwing an exception."

That sounds fine, then. And way cleaner and simpler than the stupid char buf[100]'s that I scattered everywhere.

Comment 10

[Steve Fink [:sfink] [:s:]]

It's a little ugly, because it creates a bunch of heap churn -- most things are returning a static constant string, but UniqueChars doesn't have a non-owning variant, so I guess I'll have to strdup them.

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: Remove race on TypeString() char buffers

The get() is unfortunate, but I guess it's fine.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 8915252 [details] [diff] [review]
Remove race on TypeString() char buffers

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

Great, thanks for making the updates.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4353f7c8b7
Remove race on TypeString() char buffers, r=jonco

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffd7a34aa10 for https://treeherder.mozilla.org/logviewer.html#?job_id=135865769&repo=mozilla-inbound

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1470a3142fad
Remove race on TypeString() char buffers, r=jonco

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1470a3142fad