Closed Bug 1122021 Opened 9 years ago Closed 9 years ago

TSan: data race js/src/jsnum.cpp:1098 InitRuntimeNumberState

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tsan])

Attachments

(2 files)

Attached file jsnum-race.txt
[cribbing from decoder's script, hopefully he won't mind]

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

It looks like we're setting elements of number_constants multiple times, but we're setting them to the same constant value every time.  I think we could safely get away with MOZ_TSAN_BLACKLIST here.  Or we could add an atomic sInitialized flag.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
ThreadSanitizer reports write-after-write conflicts to number_constants
while initializing JSRuntimes on different threads.  While an argument
could be made that these writes are entirely safe (they are, after all,
writing the same value every time), it seems better to move to a more
obviously not-racy solution.

This patch implements such a solution, by moving number_constants prior
to its only use in js_InitNumberClass, and forcing the initialization of
the "problematic" constants (infinities, NaNs, etc.) to take place at
static construction time.  Adding a static initializer is not ideal, but
we can't get the behavior we want from std::numeric_limits<double>:
while the infinity() and min() methods there are appropriately
constexpr, we still need the ability to select a particular NaN that
plays nicely with our Value representation.  Other alternatives, like
defining number_constants entirely in assembly with C linkage, do not
seem appealing either.  This seems like the least bad approach that
removes the data races.

In passing, this patch also removes the unused nc_slot enum.

This patch passes local compilation (Linux64); it really needs a tryserver run
to ensure that all our compilers are happy with it.
Attachment #8569235 - Flags: review?(jorendorff)
Comment on attachment 8569235 [details] [diff] [review]
make number_constants internal to js_InitNumberClass

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

r=me, thanks.

::: js/src/jsnum.cpp
@@ +1147,5 @@
> +    /*
> +     * Our NaN must be one particular canonical value, because we rely on NaN
> +     * encoding for our value representation.  See Value.h.
> +     */
> +    static JSConstDoubleSpec number_constants[] = {

Can we just remove the 'static' here? JS_DefineConstDoubles doesn't need it to be static.

This function runs each time a web page uses Number for the first time. It's not super hot. We can afford to build this little array on the stack each time this code runs.

If you think a static initializer is better overall, that's fine with me too.
Attachment #8569235 - Flags: review?(jorendorff) → review+
I remembered that |static| aggregates like that inside a function are only initialized when the function is called, so leaving it static is not really that big a deal.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56534e335e86
Assignee: nobody → nfroyd
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/56534e335e86
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: