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: nobody → kinetik
Status: NEW → ASSIGNED
This applies against Nightly + bug 1131768.
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.
Attachment #8562463 - Flags: review?(padenot)
Attachment #8562463 - Flags: review?(padenot) → review+
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
(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.
(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.
: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)
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
> [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)
(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.
The use of atomics will be removed in bug 1136360.
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 . That is, assuming OSX 10.6 doesn't throw a spanner in the works - it's not clear to me how that relates.  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.