Closed Bug 1349912 Opened 7 years ago Closed 7 years ago

Solve MinGW's std::thread problems

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file, 3 obsolete files)

MinGW has problems with std::thread. It has two threading models: win32 API based, which disables std::thread; and POSIX based which enables it but requires an emulation library (winpthreads).

While a little bit ago in Bug 1317642 we were hoping to avoid this problem and hope no one used std::thread, it seems to be coming up more and more and we won't be able to remove them all. For example: Bug 1344909 uses it (although we could work around it) and a recent cubeb update in Bug 1341238 uses it and there doesn't seem to be a non-intrusive cutover there.

The options seemed to be having libgcc/stdc++ support win32 threads natively or switching to POSIX-based threading in MinGW with pthread emulation.  The latter is, as far as I can tell, the only realistic option.
(In reply to Tom Ritter [:tjr] from comment #0)
> The options seemed to be having libgcc/stdc++ support win32 threads natively
> or switching to POSIX-based threading in MinGW with pthread emulation.  The
> latter is, as far as I can tell, the only realistic option.

I agree. I was hoping to see this fixed on GCC side for years, but it never happen and I failed to find the time to do that myself. We need to use pthead.
It looks like in Bug 1347866 Angle started using std::future (https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/WorkerThread.h) - which might not even be usable once we fix the threading problems...
See Also: → 1363216
Per talk with Henry, he could take this bug.
Assignee: nobody → hchang
Hey Tom,

I haven't got a window environment to actually look into this bug
but after a quick investigation on it, it looks like the recent MinGW
already comes with pthread support [1]. Have you already known this but
been unable to take it for some reason?

[1] http://stackoverflow.com/questions/29947302/meaning-of-options-in-mingw-w64-installer
Attached file working-script-partial.sh (obsolete) —
(In reply to Henry Chang [:hchang] from comment #4)
> Hey Tom,
> 
> I haven't got a window environment to actually look into this bug
> but after a quick investigation on it, it looks like the recent MinGW
> already comes with pthread support [1]. Have you already known this but
> been unable to take it for some reason?

So this is for a MinGW cross-compile - compiling for Windows on Linux. So it's not a matter of selecting the right option when setting up MinGW. I have most of a script that replicates the MinGW build in Docker right now, but I ran into a couple of errors I hadn't encountered before because I haven't been using Docker recently.  

Getting solutions (even temporary workarounds) for Bug 1364560 and Bug 1363506 is going to be necessary to enable a complete MinGW build, and those will probably block progress on this bug.  

In the interim, the type of thing we're looking at is the partial script I've attached. If you complete it, you'll get build errors, but you'll at least understand the environment we're working in.
Attached file working-script.sh (obsolete) —
Okay, this new script will create a complete build toolchain, check out central, and apply the appropriate patches to complete the mingw compilation.
Attachment #8867338 - Attachment is obsolete: true
Hi Tom,

Sorry i haven't got spare cycles to work on this bug. To help Ethan and me
figure out its priority, I am going to summarize what you mentioned before:

1) We can *always* use #ifdef __MINGW32__ to get around the MinGW build failure
   caused by std::thread (even though it's pretty annoying and sometimes intrusive)
   so this bug is not supposed to block MinGW build on windows.

2) According to comment 2, we are using more and more std::thread-dependent 
   STL libraries (i.e. future,async) and they might still not be usable even
   we fix the threading issue.

3) Bug 1364560 is also blocking this bug.

Are my summary all right or some are not quite correct?

Thanks.
Flags: needinfo?(tom)
(In reply to Henry Chang [:hchang] from comment #8)
> Hi Tom,
> 
> Sorry i haven't got spare cycles to work on this bug. To help Ethan and me
> figure out its priority, I am going to summarize what you mentioned before:
> 
> 1) We can *always* use #ifdef __MINGW32__ to get around the MinGW build
> failure
>    caused by std::thread (even though it's pretty annoying and sometimes
> intrusive)
>    so this bug is not supposed to block MinGW build on windows.

We have been able to use __MINGW32__ tricks in Bug 1341238 and Bug 1317642; but there are other uses where it is not so easy. For example https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_log.cpp#76 

Moving into the future, it will not be feasible to comment out all std::thread uses using __MINGW32__. So 'always' is incorrect.

> 2) According to comment 2, we are using more and more std::thread-dependent 
>    STL libraries (i.e. future,async) and they might still not be usable even
>    we fix the threading issue.

Once threading is fixed I believe these other STL libraries will work.
 
> 3) Bug 1364560 is also blocking this bug.

No. The patch attached to that bug will get a MinGW build working. While that bug isn't fixed yet; it doesn't stop work on this one.
Flags: needinfo?(tom)
I think I have a path forward on this...
Assignee: hchang → tom
Attached patch 1349912.patch (obsolete) — Splinter Review
This is a quick-fix hack, need to make this correct, but want to attach the patch before I lost it =)
Attachment #8905098 - Flags: review?(nfroyd)
Attachment #8898860 - Attachment is obsolete: true
Attachment #8871437 - Attachment is obsolete: true
Comment on attachment 8905098 [details]
Bug 1349912 Do not use win32 local includes for libstagefright on MinGW build

https://reviewboard.mozilla.org/r/176914/#review181888
Attachment #8905098 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/78e94797de06
Do not use win32 local includes for libstagefright on MinGW build r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/78e94797de06
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1456575
You need to log in before you can comment on or make changes to this bug.