Closed
Bug 1131788
Opened 10 years ago
Closed 10 years ago
Unable to use InterlockedAdd64 on MSVC2010 (and mingw)
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: Callek, Assigned: kinetik)
References
Details
Attachments
(1 file)
1.87 KB,
patch
|
padenot
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
So, while MSVC2010 is not supported on trunk, mingw is (aiui)
We also have MSVC2010 supported on beta currently.
Bug 698079 added that support with m-beta patch http://hg.mozilla.org/releases/mozilla-beta/rev/0411d20465b4
adding InterlockedAdd64.
per http://stackoverflow.com/questions/14603407/why-interlockedadd-is-not-available-in-vs2010 thats not even available at all in vs2010.
per IRC kinetic asked me to file a bug and he'd attach a patch to make this work (by abstracting out, and using a mutex when InterlockedAdd64 is not available). Claiming the fix is low risk and should be easily uplifted.
I have a patch queue on try with https://treeherder.mozilla.org/#/jobs?repo=try&revision=739946fdf587 using beta and MSVC2010 that demonstrates the problem.
Current MXR of beta: http://mxr.mozilla.org/comm-beta/search?string=InterlockedAdd64 shows only media/libcubeb/src/cubeb_wasapi.cpp as a user.
==
c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/media/libcubeb/src/cubeb_wasapi.cpp(471) : error C3861: 'InterlockedAdd64': identifier not found
c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/media/libcubeb/src/cubeb_wasapi.cpp(1214) : error C3861: 'InterlockedAdd64': identifier not found
==
Could those two calls not be rewritten with InterlockedExchangeAdd64?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
This applies against Nightly + bug 1131768.
Reporter | ||
Comment 3•10 years ago
|
||
pushed to try (with beta + MSVC2010 based build) https://treeherder.mozilla.org/#/jobs?repo=try&revision=9988f5c54320
That push includes two unrelated patches to make MSVC2010 work for breakages earlier in build system.
Assignee | ||
Updated•10 years ago
|
Attachment #8562463 -
Flags: review?(padenot)
Updated•10 years ago
|
Attachment #8562463 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8562463 [details] [diff] [review]
bug1131788_v0.patch
Approval Request Comment
[Feature/regressing bug #]: bug 1127213
[User impact if declined]: fails to compile on (beta) supported compiler MSVC 2010, makes life more difficult for comm-central developers
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: very low risk, uses an older API which is available on more platforms but provides the same functionality (for our purposes)
[String/UUID change made/needed]: none
Attachment #8562463 -
Flags: approval-mozilla-beta?
Attachment #8562463 -
Flags: approval-mozilla-aurora?
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•10 years ago
|
Attachment #8562463 -
Flags: approval-mozilla-beta?
Attachment #8562463 -
Flags: approval-mozilla-beta+
Attachment #8562463 -
Flags: approval-mozilla-aurora?
Attachment #8562463 -
Flags: approval-mozilla-aurora+
Comment 7•10 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e7a9e606aa
Please fix your commit information to include your email address in the future.
Comment 8•10 years ago
|
||
status-firefox37:
--- → fixed
Comment 9•10 years ago
|
||
status-firefox36:
--- → fixed
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #7)
> Please fix your commit information to include your email address in the
> future.
It does. Looks like this is a bug in qimportbz.
Reporter | ||
Comment 11•10 years ago
|
||
:kinetik,
So ugh's, and this issue is now blocking SeaMonkey 2.33 (see report in https://bugzilla.mozilla.org/show_bug.cgi?id=1132299#c6 )
[20:35:23] dmajor documentation for InterlockedCompareExchange64 is Vista+ so we better not be using it
[20:36:22] dmajor ...but there is a dependency from SM's xul.dll on it
[20:36:47] ewong how does Firefox 36 bypass that?
[20:37:09] dmajor by not using that function, it would seem
[20:38:43] dmajor let me see if I can find who's calling it
[20:47:53] dmajor Callek: apparently your compiler is turning the InterlockedExchangeAdd64 from bug 1131788 into InterlockedCompareExchange64
Probably deserves a new bug though (I'll file one within the day if you don't)
Flags: needinfo?(kinetik)
Reporter | ||
Comment 12•10 years ago
|
||
So per IRC convo, this actually got backed out between SeaMonkey 2.33 Beta 1 and now... so not an issue with us (right now)
https://hg.mozilla.org/releases/mozilla-beta/rev/e62dd2ded2fc
Flags: needinfo?(kinetik)
Comment 13•10 years ago
|
||
> [20:47:53] dmajor Callek: apparently your compiler is turning the InterlockedExchangeAdd64 from bug 1131788 into InterlockedCompareExchange64
InterlockedExchangeAdd64 is implemented on top of InterlockedCompareExchange64 and that API call is only available on Vista and above. You can use the complier intrinsic _InterlockedExchangeAdd64 .
Also see MBFT atomics: http://mxr.mozilla.org/comm-central/source/mozilla/mfbt/Atomics.h?rev=cfe66af4d3c8&mark=627-627,639-642#625
> Probably deserves a new bug though (I'll file one within the day if you don't)
Flags: needinfo?(kinetik)
Comment 14•10 years ago
|
||
(In reply to Philip Chee from comment #13)
The MBFT atomics are only defined for x64. In any case, InterlockedExchangeAdd64 does the right thing (generates inline machine code) with VS2013.
Assignee | ||
Comment 15•10 years ago
|
||
The use of atomics will be removed in bug 1136360.
Flags: needinfo?(kinetik)
Comment 16•10 years ago
|
||
FWIW, VS2013 also has full support for std::atomic now, and that's what mozilla::Atomic should be using under the hood. In fact since we dropped support for GCC 4.6 on Gecko 38 and up, the only remaining reason for the custom implementation in mfbt/Atomics.h might have just gone away [1]. That is, assuming OSX 10.6 doesn't throw a spanner in the works - it's not clear to me how that relates.
[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Atomics.h#33
You need to log in
before you can comment on or make changes to this bug.
Description
•