Open Bug 862657 Opened 7 years ago Updated 6 years ago

Fix failures on Mac SpiderMonkey jobs

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

People

(Reporter: philor, Assigned: Waldo)

References

Details

(Whiteboard: [leave open])

Attachments

(4 files)

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.
No takers? Shut them off now?
I'll take a look, sigh.
Attached patch Possible patchSplinter Review
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
Assignee: general → jwalden+bmo
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".
Attachment #745454 - Flags: review?(nfroyd)
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.
Attachment #745455 - Flags: review?(wmccloskey)
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.
Attachment #745454 - Flags: feedback?(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?
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 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.
Attachment #745452 - Flags: review?(nfroyd) → review+
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.
Attachment #745454 - Flags: review?(nfroyd) → review+
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.
Attachment #745455 - Flags: review?(wmccloskey) → review+
Attachment #745454 - Flags: feedback?(luke)
It sounds like this may just be due to using the wrong version of clang for the osx spidermonkey builds.
Depends on: 895208
Depends on: 923882
You need to log in before you can comment on or make changes to this bug.