Unable to use InterlockedAdd64 on MSVC2010 (and mingw)

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Callek, Assigned: kinetik)

Tracking

unspecified
mozilla38
x86_64
Windows 8.1
Points:
---

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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

4 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
This applies against Nightly + bug 1131768.
(Reporter)

Comment 3

4 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

4 years ago
Attachment #8562463 - Flags: review?(padenot)
Attachment #8562463 - Flags: review?(padenot) → review+
(Assignee)

Comment 5

4 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?
https://hg.mozilla.org/mozilla-central/rev/a2e7a9e606aa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8562463 - Flags: approval-mozilla-beta?
Attachment #8562463 - Flags: approval-mozilla-beta+
Attachment #8562463 - Flags: approval-mozilla-aurora?
Attachment #8562463 - Flags: approval-mozilla-aurora+
(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.
(Assignee)

Comment 10

4 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

4 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

4 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

4 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)
(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

4 years ago
The use of atomics will be removed in bug 1136360.
Flags: needinfo?(kinetik)
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.