Fix failures on Mac SpiderMonkey jobs

NEW
Assigned to

Status

()

Core
JavaScript Engine
5 years ago
4 years ago

People

(Reporter: philor, Assigned: Waldo)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
No takers? Shut them off now?
I'll take a look, sigh.
Created attachment 745377 [details] [diff] [review]
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
Created attachment 745452 [details] [diff] [review]
Implement mozilla::IsConst and mozilla::IsVolatile, needed for later patches
Attachment #745452 - Flags: review?(nfroyd)
Assignee: general → jwalden+bmo
Created attachment 745454 [details] [diff] [review]
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".
Attachment #745454 - Flags: review?(nfroyd)
Created attachment 745455 [details] [diff] [review]
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.
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)

Comment 8

5 years ago
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+
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.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/33a510f4ae92
https://hg.mozilla.org/mozilla-central/rev/75f047d8b347
https://hg.mozilla.org/mozilla-central/rev/83ed83e58666

Updated

5 years ago
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
(Reporter)

Updated

4 years ago
Depends on: 923882
You need to log in before you can comment on or make changes to this bug.