1141583 - TSan: data race js/src/jit/CodeGenerator.cpp:1771 emitLambdaInit

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: emit-lambda-init-race.txt

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

* Specific information about this bug

I apologize for the lack of stack on the writing side of things; based on code inspection, I assume we must be writing here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#7342
http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#7351

creating the LambdaFunctionInfo in MLambdaArrow's constructor via IonBuilder::jsop_lambda_arrow.  But beyond that...

I note in passing the comment in LambdaFunctionInfo:

    // The functions used in lambdas are the canonical original function in
    // the script, and are immutable except for delazification. Record this
    // information while still on the main thread to avoid races.

...is not enough to ensure that the code is race-free. :)

* 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

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Can you see if this patch helps? fun->nargs() should be immutable, but it's an uint16_t so it's possible the flag bits (also an uint16_t) are modified.

This patch just adds nargs to LambdaFunctionInfo to avoid reading fun->nargs() offthread.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Can you see if this patch helps? fun->nargs() should be immutable, but it's
> an uint16_t so it's possible the flag bits (also an uint16_t) are modified.
> 
> This patch just adds nargs to LambdaFunctionInfo to avoid reading
> fun->nargs() offthread.

My copy of CodeGenerator.cpp and the given line information says that the race comes from the access to flags:

http://hg.mozilla.org/mozilla-central/file/c42e7e3bb0a0/js/src/jit/CodeGenerator.cpp#l1771

Since the race access size is size 2 in both cases, I don't think we're doing anything fancy with nargs and flags being separate fields...

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> My copy of CodeGenerator.cpp and the given line information says that the
> race comes from the access to flags:

Hm then I don't understand this race. Is there any way you can get a stack trace for the write on the main thread?

IonBuilder constructs the MIR graph on the main thread. This includes filling LambdaFunctionInfo. Then this graph is compiled off-thread and that's where the read happens.

I doubt it's complaining about the initial LambdaFunctionInfo::flags write on the main thread and read of it off-thread, because that's what we do for basically all MIR instructions...

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8575358 [details] [diff] [review]
Patch

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

I don't see the race after two runs with this patch applied, so I think it might have gone away...?

The line information thing still seems a little weird, though...

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Store nargs in LambdaFunctionInfo and remove an unused copy constructor.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe07d44b06b

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cfe07d44b06b