401188 - Thread-unsafe updates to sub-atomic rt->gc{Poke,Zeal}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

This came up in a conversation with Jason over IRC. The comment right above the packed gc-prefixed bools in struct JSRuntime warns against what went wrong with the patch for bug 308429. Wish I had read that when reviewing...

/be

Comment 1

[Brian Crowder]

Attachment: v1, where I should have put this in the first place.

Sorry I missed this as well.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

We should keep gc* members adjacent, if possible. But more important, do we want to interlock setting zeal with getting it? Igor, your thoughts are welcome.

/be

Comment 3

[Brian Crowder]

Won't keeping them adjacent without padding change the shape of the runtime structure in an incompatible way?

Comment 4

[Brendan Eich [:brendan]]

We don't preserve binary compatibility for JSRuntime or any other such private structs. The JS API does not export such structs. The few in jsapi.h are arguably frozen, but we have extended some over time. We really don't promise ABI compat, only API compat.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

It's worth noting that in the one case that we do have an extensible struct, there are explicit slots in which to put additional members (JSExtendedClass).

Comment 6

[Igor Bukanov]

Comment on attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

>Index: jscntxt.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jscntxt.h,v
>retrieving revision 3.165
>diff -u -p -8 -r3.165 jscntxt.h
>--- jscntxt.h	18 Sep 2007 01:22:20 -0000	3.165
>+++ jscntxt.h	26 Oct 2007 03:49:01 -0000
>@@ -187,22 +187,17 @@ struct JSRuntime {
>     /*
>      * NB: do not pack another flag here by claiming gcPadding unless the new
>      * flag is written only by the GC thread.  Atomic updates to packed bytes
>      * are not guaranteed, so stores issued by one thread may be lost due to
>      * unsynchronized read-modify-write cycles on other threads.
>      */
>     JSPackedBool        gcPoke;
>     JSPackedBool        gcRunning;
>-#ifdef JS_GC_ZEAL
>-    uint8               gcZeal;
>-    uint8               gcPadding;
>-#else
>     uint16              gcPadding;
>-#endif
> 
>     JSGCCallback        gcCallback;
>     JSGCThingCallback   gcThingCallback;
>     void                *gcThingCallbackClosure;
>     uint32              gcMallocBytes;
>     JSGCArenaInfo       *gcUntracedArenaStackTop;
> #ifdef DEBUG
>     size_t              gcTraceLaterCount;
>@@ -416,16 +411,20 @@ struct JSRuntime {
>     jsrefcount          totalStrings;
>     jsrefcount          liveDependentStrings;
>     jsrefcount          totalDependentStrings;
>     double              lengthSum;
>     double              lengthSquaredSum;
>     double              strdepLengthSum;
>     double              strdepLengthSquaredSum;
> #endif
>+
>+#ifdef JS_GC_ZEAL
>+    uint8               gcZeal;
>+#endif
> };

It is better to have something like jsrefcount as the type for gcZeal that is guranteed to be atomic. r+ with that fixed.

Comment 7

[Brian Crowder]

Attachment: With Igor's and Brendan's feedback

Comment 8

[Reed Loden [:reed]]

Checking in js/src/jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.169; previous revision: 3.168
done

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What platforms do we support where single-byte writes are non-atomic?

Comment 10

[Jesse Ruderman]

(roc wants to know for bug 404870 -- see the "nsThread fix" patch there.)

Comment 11

[Benjamin Smedberg]

on x86 single-byte writes are not atomic unless you use special atomic instructions... but a quick google search couldn't find the reference to the paper about this.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hans Boehm seems to suggest they are atomic:
http://gcc.gnu.org/ml/java/2001-01/msg00084.html
He's an expert on this stuff, but I'd like to see a real reference one way or the other...

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For what it's worth, this Microsoft article seems to agree:
http://msdn2.microsoft.com/en-us/library/bb310595.aspx
"On all modern processors, you can assume that reads and writes of naturally aligned native types are atomic."

Comment 14

[Daniel Veditz [:dveditz]]

This will be wanted on the 1.8 branch if bug 308429 is taken

Comment 15

[Brian Crowder]

Should clear the wanted/blocking flags on these bugs, as they are replaced for the 1.8 branch by the rollup patch in bug 426628.

Comment 16

[Daniel Veditz [:dveditz]]

except that the bug description there doesn't mention this problem which means it would be very easy for QA to miss verifying parts of the fix. In the case of this bug it might not matter much, but if there were a testcase here we definitely leave bugs like this on the blocking list on purpose even when fixed by a patch in another bug.