Closed
Bug 1336344
Opened 7 years ago
Closed 7 years ago
Cannot build on Linux/aarch64 after landing bug 1333429
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | fixed |
People
(Reporter: m_kato, Unassigned)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.39 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
Even if Linux, sizeof(pthread_mutex_t) might be greater than 40. Example, in /usr/include/aarch64-linux-gnu/bits/pthreadtypes.h ... #define __SIZEOF_PTHREAD_MUTEX_T 48 ... So I cannot build for Linux/aarch64 due to the following build error. 0:02.56B /hg/mozilla-central/js/src/threading/posix/MutexImpl.cpp: In member function 'js::detail::MutexImpl::PlatformData* js::detail::MutexImpl::platformData()': 0:02.56B /hg/mozilla-central/js/src/threading/posix/MutexImpl.cpp:76:3: error: static assertion failed: platformData_ is too small 0:02.56B static_assert(sizeof(platformData_) >= sizeof(PlatformData), 0:02.56B 0:02.56B 0:02.56B In the directory /hg/mozilla-central/obj-aarch64-linux-gnu/js/src
Reporter | ||
Updated•7 years ago
|
Keywords: regression
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
Reporter | ||
Comment 1•7 years ago
|
||
On Linux/aarch64, __SIZEOF_PTHREAD_MUTEX_T is 48. So current size (40) is not enough.
Attachment #8834744 -
Flags: review?(nfitzgerald)
Comment 2•7 years ago
|
||
I don't get why this is defining platformData_ as an array of void* at all. Why not just use "PlatformData platformData_"?
Flags: needinfo?(nfroyd)
Comment 3•7 years ago
|
||
I see this is actually a regression from bug 1256582 or thereabouts. The rationale for the void* trickery is laid out in bug 1256089 comment 3. The comment he refers to is: https://hg.mozilla.org/mozilla-central/file/2cecd1a0e910/js/src/threading/windows/Mutex.cpp#l17 I'm not familiar enough with the Windows headers to know whether sizeof(CRITICAL_SECTION) changes depending on whether you're compiling for XP or for Vista. (That sounds awful if it's true.) And I'm not sure whether the comment about configury is correct; I should think that pthreads is standard enough by now that we shouldn't really need care whether pthreads.h is conditionally available? But I think the goal of keeping <windows.h> includes out of the way is definitely valuable. We could just: #ifdef WIN32 // Avoid #includes of <windows.h> in header files. void* platformData_[6]; #else pthread_mutex_t platformData_; #endif with appropriate #includes. I'd prefer this be done after bug 1312086 lands, but that's just because it makes my life easier. ;)
Flags: needinfo?(nfroyd)
Comment 4•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3) > We could just: > > #ifdef WIN32 > // Avoid #includes of <windows.h> in header files. > void* platformData_[6]; > #else > pthread_mutex_t platformData_; > #endif > > with appropriate #includes. I'd prefer this be done after bug 1312086 > lands, but that's just because it makes my life easier. ;) This SGTM
Comment 5•7 years ago
|
||
Comment on attachment 8834744 [details] [diff] [review] Cannot build on Linux/aarch64 after landing bug 1333429. r?fitzgen Review of attachment 8834744 [details] [diff] [review]: ----------------------------------------------------------------- Holding off until the move to mozglue is done, then I think we should do froydnj's suggestion. Thanks!
Attachment #8834744 -
Flags: review?(nfitzgerald)
Comment 6•7 years ago
|
||
For pthreads platforms, we have a terribly large condition for the size of a MutexImpl that attempts to hardcode the number of words that pthread_mutex_t takes. This hardcoding isn't always correct. We originally did this to try and keep <pthread.h> includes out of the headers, but the PlatformConditionVariable.h header already includes <pthread.h> anyway, and <pthread.h> isn't so namespace-invasive as <windows.h>. So go ahead and include <pthread.h> and use the actual definition of pthread_mutex_t to size the platformData_ member.
Attachment #8840096 -
Flags: review?(nfitzgerald)
Updated•7 years ago
|
Attachment #8834744 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8840096 -
Flags: review?(nfitzgerald) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/999757e9e5a5 use sizeof(pthread_mutex_t) to size MutexImpl's platformData_; r=fitzgen
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/999757e9e5a5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•