TSan: data race js/src/gc/Heap.h:551:26 in getAllocKind

RESOLVED FIXED in Firefox 48



JavaScript: GC
2 years ago
2 years ago


(Reporter: jseward, Assigned: jonco)



Firefox Tracking Flags

(firefox48 fixed)


(Whiteboard: [tsan])


(2 attachments, 1 obsolete attachment)



2 years ago
Created attachment 8731255 [details]

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

       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) [1][2].

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.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf


2 years ago
Whiteboard: [tsan]

Comment 1

2 years ago
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.

Comment 2

2 years ago
Created attachment 8731610 [details] [diff] [review]

As described above plus a little refactoring.
Assignee: nobody → jcoppeard
Attachment #8731610 - Flags: review?(terrence)

Comment 3

2 years ago
Created attachment 8731656 [details] [diff] [review]

...and uploading non-empty patch this time.
Attachment #8731610 - Attachment is obsolete: true
Attachment #8731610 - Flags: review?(terrence)
Attachment #8731656 - Flags: review?(terrence)

Comment 4

2 years ago
(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]

Review of attachment 8731656 [details] [diff] [review]:

That's a nice cleanup too!
Attachment #8731656 - Flags: review?(terrence) → review+

Comment 7

2 years ago
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.