All Tier3 OS need some plumbing. Bert, how Tier1 got those numbers? Why not sizeof(pthread_mutex_t) ? In file included from js/src/threading/posix/Mutex.cpp:11: js/src/threading/Mutex.h:45:2: error: "Mutex platform data size isn't known for this platform" #error "Mutex platform data size isn't known for this platform" ^ js/src/threading/posix/Mutex.cpp:43:24: error: unknown type name 'platformData_'; did you mean 'PlatformData'? static_assert(sizeof(platformData_) >= sizeof(PlatformData), ^~~~~~~~~~~~~ PlatformData js/src/threading/posix/MutexPlatformData.h:16:15: note: 'PlatformData' declared here struct Mutex::PlatformData ^ js/src/threading/posix/Mutex.cpp:45:42: error: use of undeclared identifier 'platformData_' return reinterpret_cast<PlatformData*>(platformData_); ^ 3 errors generated.

Runtime tested only on FreeBSD 11.0C amd64. As I have a few VMs/jails lying around (but nothing Solaris-like), here're more sizeof(pthread_mutex_t) numbers: 8 - DragonFly 8 - FreeBSD 64bit 4 - FreeBSD 32bit 8 - OpenBSD 64bit 40 - GNU/kFreeBSD 64bit 48 - NetBSD 64bit 28 - NetBSD 32bit

This is pretty much unworkable this way. If sizeof(pthread_mutex_t) does not work, it has to be queried at configure time. The size is machine dependend on NetBSD, various paddings may apply, and one of the early members is a __cpu_simple_lock_t, which varies from a single byte to 64bits, depending on the cpu.

Comment on attachment 8729905 [details] [diff] [review] v0 Review of attachment 8729905 [details] [diff] [review]: ----------------------------------------------------------------- Right, so there are some serious constraints here. First, the distribution model on OSX and Windows is such that you build for one platform and distribute to all other platforms. This means that the pthread_mutex_t or CRITICAL_SECTION definition we configure and build with may be different from the ones used at runtime. This wouldn't normally be a constraint except that we actually need to set a field that isn't in the headers we build against, because windows [1]. Secondly, Window's kitchen-sink approach to headers means that we cannot just #include <Windows.h> from a header file without conflicting all the symbols. We could clearly do so for posix and just special case Windows, however, Mozilla's configuration system is such that SpiderMonkey doesn't actually inherit any of Firefox's build configuration, so we'd have to duplicate the pthreads detection code into SpiderMonkey's configury. Normally, I'd have done this except that two weeks ago the build teams started moving everything in configure over to a new (as yet undocumented and largely imaginary) system. Luckily, there's no penalty for making the platform data too big, so I think for tier 3 support, in the near term, we should just make the default blob large enough to satisfy the majority of platforms and expand it as we find larger ones. I'll post a patch. 1- https://dxr.mozilla.org/mozilla-central/source/js/src/threading/windows/Mutex.cpp#17-22 ::: js/src/threading/Mutex.h @@ +41,3 @@ > void* platformData_[40 / sizeof(void*)]; > +#elif defined(__NetBSD__) > + void* platformData_[sizeof(pthread_mutex_t) / sizeof(void*)]; This would need a #include <pthread.h> at the top and would need to be protected by HAVE_PTHREAD or equivalent. This probably only works currently because the posix-nspr-wrapper tests are being built with the Mutex tests because of unified and is not something we can count on.

Jan, does this patch work for you?

Comment on attachment 8730273 [details] [diff] [review] 00_mutex__04_support_other_platforms-v0.diff Works fine. Is / sizeof(void*) supposed to bloat size on 32bit platforms and shrink on 128bit?

(In reply to Jan Beich from comment #5) > Comment on attachment 8730273 [details] [diff] [review] > 00_mutex__04_support_other_platforms-v0.diff > > Works fine. Is / sizeof(void*) supposed to bloat size on 32bit platforms and > shrink on 128bit? The opposite, actually. The type of platformData_ is void*[], so if we know the struct is exactly 6 pointers, e.g. in windows, then we can get away with a single |void* platformData_[6];| declaration and don't need to specialize for machine widths. On the other hand, if the structure is not purely pointers, we want to have a fixed size and need to divide the number of bytes we're requesting by the size of the type stored in the array: void*.

Comment on attachment 8730273 [details] [diff] [review] 00_mutex__04_support_other_platforms-v0.diff I'll ask in #build about how to get HAVE_PTHREAD defined in SpiderMonkey builds. That's probably going to take awhile though, so in the meantime, I think we should land this so that our Tier-3 maintainers can get on with their lives.

The reason I added the void* array is indeed, as Terrence said, so we can sometimes get away with a fixed number of CPU words independent of the processor architecture. There's no harm in over-allocating a little bit, although it seems that many BSDs need only one pointer-size word. > 8 - DragonFly > 8 - FreeBSD 64bit > 4 - FreeBSD 32bit > 8 - OpenBSD 64bit > 40 - GNU/kFreeBSD 64bit > 48 - NetBSD 64bit > 28 - NetBSD 32bit So maybe a solution would be: #elif defined(__DragonFly__) || defined(__FreeBSD__) || defined(__OpenBSD__) void* platformData_[1]; #else void* platformData_[7]; #endif - Bert

Comment on attachment 8730273 [details] [diff] [review] 00_mutex__04_support_other_platforms-v0.diff Review of attachment 8730273 [details] [diff] [review]: ----------------------------------------------------------------- WFM.

(In reply to Bert from comment #8) > So maybe a solution would be: > > #elif defined(__DragonFly__) || defined(__FreeBSD__) || defined(__OpenBSD__) > void* platformData_[1]; > #else > void* platformData_[7]; > #endif Looks good - maybe we can add someplace else (in a test case?), where we can do the full include dance, a compile time assert to catch mistakes with this calculation?

Ugh, between this and bug 1256582, it looks like we're really not going to be able to get away without having the pthread platform definitions available at build time. Now that I'm thinking it through, I think that pthread.h should be available everywhere that is #ifndef XP_WIN. There are platforms that are not windows that don't have pthreads, but they're probably already having to carry a pile of hacks to make themselves work. Maybe something like: #ifndef XP_WIN # include <pthread.h> # define HAVE_PTHREADS 1 #endif ... Mutex definition ... #ifdef HAVE_PTHREADS void* platformData_[sizeof(pthread_foo_t) / sizeof(void*)]; #else void* platformData_[6]; #endif

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cecd1a0e9107a2d29e1a485c351a7cd783e96e7 Bug 1256089 - Fix Mutex support for tier-3 platforms; r=froydnj

Actually, lets do followup in a new bug. Moving to bug 1256713.

Err, wrong bug on that try build.