Closed Bug 1305005 Opened 5 years ago Closed 4 years ago

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


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




Tracking Status
firefox58 --- fixed


(Reporter: jseward, Assigned: sfink)


(Keywords: triage-deferred)


(3 files, 1 obsolete file)

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.
Attached file TSan complaint
TSan also detects the write-write race on |bufs[which][.]|.
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?
Flags: needinfo?(sphink)
Keywords: triage-deferred
Priority: -- → P3
(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.
Flags: needinfo?(sphink)
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.
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.
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.
Attachment #8914528 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
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.
Attachment #8914528 - Flags: review?(jcoppeard)
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.
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.
The get() is unfortunate, but I guess it's fine.
Attachment #8915252 - Flags: review?(jcoppeard)
Attachment #8914528 - Attachment is obsolete: true
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.
Attachment #8915252 - Flags: review?(jcoppeard) → review+
Pushed by
Remove race on TypeString() char buffers, r=jonco
Pushed by
Remove race on TypeString() char buffers, r=jonco
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.