862657 - Fix failures on Mac SpiderMonkey jobs

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Phil Ringnalda (:philor)]

Because I'm not shy about just taking them away from you if you leave them busted too long.

warnaserr: https://tbpl.mozilla.org/php/getParsedLog.php?id=21886474&tree=Mozilla-Inbound
dtrace: https://tbpl.mozilla.org/php/getParsedLog.php?id=21886262&tree=Mozilla-Inbound

jsgc.cpp
In file included from /builds/slave/m-in_osx64-d_sm-warnaserrdebug/src/js/src/jsgc.cpp:50:
In file included from ../../src/js/src/jsatom.h:22:
In file included from ../../src/js/src/gc/Barrier.h:12:
../../src/js/src/gc/Heap.h:664:8: error: cannot initialize a parameter of type 'void *' with an rvalue of type 'volatile uintptr_t (*)[2016]'
struct ChunkBitmap
       ^~~~~~~~~~~
/builds/slave/m-in_osx64-d_sm-warnaserrdebug/src/js/src/jsgc.cpp:3099:9: note: in instantiation of function template specialization 'js::Swap<js::gc::ChunkBitmap>' requested here
        js::Swap(*entry, *bitmap);
        ^
/builds/slave/m-in_osx64-d_sm-warnaserrdebug/objdir/dist/include/js/Utility.h:737:7: note: implicit default copy assignment operator for 'js::gc::ChunkBitmap' first required here
    t = Move(u);
      ^
1 error generated.

They've been busted for a long long time, so there may well be others below that one, though.

Comment 1

[Phil Ringnalda (:philor)]

No takers? Shut them off now?

Comment 2

[Jeff Walden [:Waldo]]

I'll take a look, sigh.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Possible patch

As far as I can tell, I can't test these specific builds on try, but at least I can test that the patch compiles on everything else, then if it does I can push and see what happens.  This adds pod-operation gnarliness to allow volatile to flow places, and may be enough to fix things.  We'll find out.

https://tbpl.mozilla.org/?tree=Try&rev=98f21cac994d

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::IsConst and mozilla::IsVolatile, needed for later patches

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Make PodCopy work copying to volatile addresses (when operator= works), add PodArrayCopy for copying fixed-size arrays

You can't memset, memcpy, etc. volatile objects.  The compiler has to be able to see the actual expressions/operations used to modify that memory.  That motivates these overloads that work on volatile data, but don't include any memset or equivalent "optimizations".

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Speculative fix for Swap<ChunkBitmap>(), by giving ChunkBitmap a Move constructor

This might or might not work, but I believe the change at least won't hurt anything, so it seems worth trying to see what happens.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 745454 [details] [diff] [review]
Make PodCopy work copying to volatile addresses (when operator= works), add PodArrayCopy for copying fixed-size arrays

Luke, your comments here would also be appreciated, I think.

I think eventually we probably want to get rid of volatile entirely for this junk, and use actual atomic classes/arrays of them, maybe.  But in the meantime, this is maybe the most reasonable thing, I think.

Comment 8

[Luke Wagner [:luke]]

I would think the normal overload resolution rules would allow you to just add the volatile overload without requiring the EnableIf; is that not the case?

Comment 9

[Jeff Walden [:Waldo]]

Hmm.  It seems to be!  Possibly I added that at some intermediate state when it might have been needed, but in the final state it was superfluous.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 745452 [details] [diff] [review]
Implement mozilla::IsConst and mozilla::IsVolatile, needed for later patches

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

I think this is fine to go in as-is, even though it sounds like IsVolatile isn't needed for part 2.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 745454 [details] [diff] [review]
Make PodCopy work copying to volatile addresses (when operator= works), add PodArrayCopy for copying fixed-size arrays

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

r=me with removing the #include of TypeTraits.h and the EnableIf bit for the non-volatile PodCopy if that all compiles as expected.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 745455 [details] [diff] [review]
Speculative fix for Swap<ChunkBitmap>(), by giving ChunkBitmap a Move constructor

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

Thanks Jeff.

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33a510f4ae92
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f047d8b347
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ed83e58666

Unfortunately that seems to have not affected the error.  I'll look some more, I guess.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33a510f4ae92
https://hg.mozilla.org/mozilla-central/rev/75f047d8b347
https://hg.mozilla.org/mozilla-central/rev/83ed83e58666

Comment 15

[Steve Fink [:sfink] [:s:]]

It sounds like this may just be due to using the wrong version of clang for the osx spidermonkey builds.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.