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)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tsan])
Attachments
(2 files)
11.58 KB,
text/plain
|
Details | |
5.39 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
[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
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
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
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•