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)

Unspecified
Linux
defect
Not set
normal

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)

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
Keywords: regression
On Linux/aarch64, __SIZEOF_PTHREAD_MUTEX_T is 48.  So current size (40) is not enough.
Attachment #8834744 - Flags: review?(nfitzgerald)
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)
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)
(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 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)
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)
Attachment #8834744 - Attachment is obsolete: true
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
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.