Created attachment 8731255 [details] race-getAllocKind The attached logfile shows a data race detected by TSan (ThreadSanitizer). * Specific information about this bug This looks like a bitfield race, on fields in the class Arena in js/src/gc/Heap.h. Specifically, the following fields are packed into a single machine word size_t allocKind : 8; size_t hasDelayedMarking : 1; size_t allocatedDuringIncremental : 1; size_t markOverflow : 1; size_t auxNextLink : JS_BITS_PER_WORD - 8 - 1 - 1 - 1; auxNextLink will have size 21 or 53 bits on a 32 or 64 bit platform, respectively. Assignments to auxNextLink cannot be done atomically and necessarily involve a read-modify-write of the entire word, which includes the preceding 4 fields. Hence the following apparently harmless fragment T1: allocKind = X T2: auxNextLink = Y can lead to the allocKind assignment being lost, in the following interleaving: T1 T2 uint32_t temp = (read containing word) allocKind = X (8 bit atomic store to one byte of the word) temp = temp & ~MASK temp = temp | ((X << SHIFT) & MASK) (write containing word) = temp * General information about TSan, data races, etc. 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) . 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.  http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong  _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
We can change the way compacting works to not use the auxNextLink pointer by handing list of arenas to background update tasks as (first, list) arena pointer pairs. This would make the thread coordination less efficient as we wouldn't be able to have a list encompassing more than one arena kind (we have to traverse using the arena's next pointer) but I don't think that would make much difference in practice.
Created attachment 8731610 [details] [diff] [review] bug1257186-compacting-race As described above plus a little refactoring.
Assignee: nobody → jcoppeard
Attachment #8731610 - Flags: review?(terrence)
Created attachment 8731656 [details] [diff] [review] bug1257186-compacting-race ...and uploading non-empty patch this time.
(In reply to Jon Coppeard (:jonco) from comment #3) > bug1257186-compacting-race > ...and uploading non-empty patch this time. This does appear to get rid of the races, yes! +1 from me.
Comment on attachment 8731656 [details] [diff] [review] bug1257186-compacting-race Review of attachment 8731656 [details] [diff] [review]: ----------------------------------------------------------------- That's a nice cleanup too!
Attachment #8731656 - Flags: review?(terrence) → review+
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.