Solve MinGW's std::thread problems

RESOLVED FIXED in Firefox 57

Status

()

Core
General
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [tor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 months ago
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.

Comment 1

7 months ago
(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.
(Assignee)

Comment 2

5 months ago
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...
(Assignee)

Updated

5 months ago
See Also: → bug 1363216
Per talk with Henry, he could take this bug.
Assignee: nobody → hchang

Comment 4

5 months ago
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
Another reference just for notes:
http://stackoverflow.com/questions/37358856/does-mingw-w64-support-stdthread-out-of-the-box-when-using-the-win32-threading
(Assignee)

Comment 6

5 months ago
Created attachment 8867338 [details]
working-script-partial.sh

(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.
(Assignee)

Comment 7

5 months ago
Created attachment 8871437 [details]
working-script.sh

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

Comment 8

4 months ago
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)
(Assignee)

Comment 9

4 months ago
(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)
(Assignee)

Comment 10

3 months ago
I think I have a path forward on this...
Assignee: hchang → tom
(Assignee)

Comment 11

2 months ago
Created attachment 8898860 [details] [diff] [review]
1349912.patch

This is a quick-fix hack, need to make this correct, but want to attach the patch before I lost it =)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8905098 - Flags: review?(nfroyd)
(Assignee)

Updated

a month ago
Attachment #8898860 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8871437 - Attachment is obsolete: true

Comment 13

a month ago
mozreview-review
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+
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 14

a month ago
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
Last Resolved: a month ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox55: affected → ---
You need to log in before you can comment on or make changes to this bug.