1131788 - Unable to use InterlockedAdd64 on MSVC2010 (and mingw)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Justin Wood (:Callek)]

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 
==

Comment 1

[(Away)]

Could those two calls not be rewritten with InterlockedExchangeAdd64?

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: bug1131788_v0.patch

This applies against Nightly + bug 1131768.

Comment 3

[Justin Wood (:Callek)]

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.

Comment 4

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e7a9e606aa

Comment 5

[Matthew Gregan [:kinetik]]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2e7a9e606aa

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/52263e093c85

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57f79302248f

Comment 10

[Matthew Gregan [:kinetik]]

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

Comment 11

[Justin Wood (:Callek)]

: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)

Comment 12

[Justin Wood (:Callek)]

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

Comment 13

[Philip Chee]

> [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)

Comment 14

[(Away)]

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

Comment 15

[Matthew Gregan [:kinetik]]

The use of atomics will be removed in bug 1136360.

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

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