Bug 106740 – Fix the atomicIncrement implementation for Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Csaba Osztrogonác]

After https://trac.webkit.org/changeset/139514 we need 32 and 64 bit implementation 
for atomicIncrement(). I fixed it for GCC in http://trac.webkit.org/changeset/139553,
but we still need fix for Windows too:
----------------------------------------------------------------------------------------------------------------------------
#if COMPILER(MINGW) || COMPILER(MSVC7_OR_LOWER) || OS(WINCE)
inline int atomicIncrement(int* addend) { return InterlockedIncrement(reinterpret_cast<long*>(addend)); }
inline int atomicDecrement(int* addend) { return InterlockedDecrement(reinterpret_cast<long*>(addend)); }
#else
inline int atomicIncrement(int volatile* addend) { return InterlockedIncrement(reinterpret_cast<long volatile*>(addend)); }
inline int atomicDecrement(int volatile* addend) { return InterlockedDecrement(reinterpret_cast<long volatile*>(addend)); }
#endif
----------------------------------------------------------------------------------------------------------------------------

Maybe the InterlockedIncrement64 and InterlockedDecrement64 would be 
the fix, but I'm afraid it isn't implemented on 32 bit MinGW platform.

[Zoltan Arvai]

After trying build with InterlockedIncrement64 and InterlockedDecrement64, it seems it is not supported on 32bit build :(
Msdn has some resources about it: http://msdn.microsoft.com/en-us/library/windows/desktop/2ddez55b%28v=vs.85%29.aspx


diff --git a/Source/WTF/wtf/Atomics.h b/Source/WTF/wtf/Atomics.h
index 86ed805..7fc56a1 100644
--- a/Source/WTF/wtf/Atomics.h
+++ b/Source/WTF/wtf/Atomics.h
@@ -90,6 +90,9 @@ inline int atomicDecrement(int* addend) { return InterlockedDecrement(reinterpre
#else
inline int atomicIncrement(int volatile* addend) { return InterlockedIncrement(reinterpret_cast<long volatile*>(addend)); }
inline int atomicDecrement(int volatile* addend) { return InterlockedDecrement(reinterpret_cast<long volatile*>(addend)); }
+
+inline int64_t atomicIncrement(int64_t volatile* addend) { return InterlockedIncrement64(reinterpret_cast<long volatile*>(addend)); }
+inline int64_t atomicDecrement(int64_t volatile* addend) { return InterlockedDecrement64(reinterpret_cast<long volatile*>(addend)); }
#endif
#elif OS(DARWIN)
 

Error message: 

LLIntOffsetsExtractor.cpp
C:\WebKitBuildSlave\proba\WebKit\Source\WTF\wtf/Atomics.h(94) : error C3861: 'InterlockedIncrement64': identifier not found
C:\WebKitBuildSlave\proba\WebKit\Source\WTF\wtf/Atomics.h(95) : error C3861: 'InterlockedDecrement64': identifier not found
NMAKE : fatal error U1077: '"c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\cl.EXE"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: 'cd' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2'
Stop.

[Csaba Osztrogonác]

It is a bad news. :( Simon, Jocelyn, have you got any better idea? 

Should we disable building WebKit2 on the Windows bot to test the 
WK1 build at least until you find the proper fix for it?

[Csaba Osztrogonác]

ping?

[Zoltan Arvai]

I found some resources about "64-bit atomic reads and writes on x86":
http://www.niallryan.com/node/137

Currently the site is down, but web archive saved it for the future:
http://web.archive.org/web/20120418112056/http://www.niallryan.com/node/137

There is an atomic increment implementation example in the comments (by niall), but also this commenter refers to InterlockedIncrement64 on Windows. Maybe we just missed something.

[Csaba Osztrogonác]

I think we need something similar to GCC does.

Here is a small example:
-------------------------
long long a,b;
int main() {
    a = 0xDEADBEEF;
    b = __sync_add_and_fetch(&a, 0x11111111);
    return b;
}


and the assembly generated by GCC:
-----------------------------------

00000000 <main>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   57                      push   %edi
   4:   56                      push   %esi
   5:   53                      push   %ebx
   6:   83 ec 04                sub    $0x4,%esp
   9:   c7 05 00 00 00 00 ef    movl   $0xdeadbeef,0x0
  10:   be ad de
  13:   c7 05 04 00 00 00 00    movl   $0x0,0x4
  1a:   00 00 00
  1d:   a1 00 00 00 00          mov    0x0,%eax
  22:   8b 15 04 00 00 00       mov    0x4,%edx
  28:   89 c1                   mov    %eax,%ecx
  2a:   89 d3                   mov    %edx,%ebx
  2c:   81 c1 11 11 11 11       add    $0x11111111,%ecx
  32:   83 d3 00                adc    $0x0,%ebx
  35:   89 ce                   mov    %ecx,%esi
  37:   89 df                   mov    %ebx,%edi
  39:   89 0c 24                mov    %ecx,(%esp)
  3c:   89 d9                   mov    %ebx,%ecx
  3e:   8b 1c 24                mov    (%esp),%ebx
  41:   f0 0f c7 0d 00 00 00    lock cmpxchg8b 0x0
  48:   00
  49:   75 dd                   jne    28 <main+0x28>
  4b:   89 f0                   mov    %esi,%eax
  4d:   89 fa                   mov    %edi,%edx
  4f:   a3 00 00 00 00          mov    %eax,0x0
  54:   89 15 04 00 00 00       mov    %edx,0x4
  5a:   a1 00 00 00 00          mov    0x0,%eax
  5f:   8b 15 04 00 00 00       mov    0x4,%edx
  65:   83 c4 04                add    $0x4,%esp
  68:   5b                      pop    %ebx
  69:   5e                      pop    %esi
  6a:   5f                      pop    %edi
  6b:   5d                      pop    %ebp
  6c:   c3                      ret

[Zoltan Arvai]

I tried a dirty implementation in Atomics.h, based on the linked blog content.
It builds fine, QtTestBrowser seems ok, MiniBrowser runs with strange duplicated output - but this can be caused by remote desktop connection that I use.


inline int64_t atomicIncrement(int64_t volatile* addend)
{ 
	// return InterlockedIncrement64(reinterpret_cast<long volatile*>(addend)); 
	__asm
	{
		mov edi, addend
		mov eax, [edi]      //read current value non-atomically here...
		mov edx, [edi+4]    //  it's just a guess, if it's wrong we'll try again
	tryAgain:
		mov ebx, 1
		mov ecx, 0
		add ebx, eax
		adc ecx, edx
		lock cmpxchg8b qword ptr [edi]
		jnz tryAgain
	}
	return *addend;
}

[Csaba Osztrogonác]

QtTestBrowser is absolutely irrelevant, because 64-bit atomicIncrement
is only used by WebKit2. Please try it with MiniBrowser without rdesktop.

[Jocelyn Turcotte]

(In reply to comment #1)
> After trying build with InterlockedIncrement64 and InterlockedDecrement64, it seems it is not supported on 32bit build :(
> Msdn has some resources about it: http://msdn.microsoft.com/en-us/library/windows/desktop/2ddez55b%28v=vs.85%29.aspx

There:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683615(v=vs.85).aspx

They say:
"This function is implemented using a compiler intrinsic where possible. For more information, see the WinBase.h header file and _InterlockedIncrement64."

The 64bit intrinsic isn't available on 32bit like said in the page you refer, but this should mean that windows.h should export this function anyway and implement it without the intrinsic. But that doesn't fit with the error you get.

[Zoltan Arvai]

(In reply to comment #7)
> QtTestBrowser is absolutely irrelevant, because 64-bit atomicIncrement
> is only used by WebKit2. Please try it with MiniBrowser without rdesktop.

MiniBrowser works fine on normal desktop.

I also tried a build just before r139514 and MiniBrowser behaves the same way. Maybe the angle lib does not like remote desktop.

So tomorrow we can fix this build issue finally :)

[Zoltan Arvai]

Created attachment 183457 [details]
patch

let see ews results...

[WebKit Review Bot]

Attachment 183457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/At..." exit_code: 1
Source/WTF/wtf/Atomics.h:88:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/Atomics.h:92:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/Atomics.h:101:  Extra space before [  [whitespace/braces] [5]
Source/WTF/wtf/Atomics.h:110:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WTF/wtf/Atomics.h:119:  Extra space before [  [whitespace/braces] [5]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Arvai]

Created attachment 183779 [details]
patch moving support to xpsp2 and later

In the end of last week we had a conversation on #qtwebkit with Jocelyn to see what options are available for fixing WebKit2 build on Windows. MSVC's InterlockedIncrement64 method is available from XPSP2. This patch moves support from 2000 to XPSP2 and later versions of Windows. Let see what is EWS' opinion about this. On my test machine it seems to be ok.

The other way is to implement the required atomicIncrement functionality in assembly for (current) older target Windows target version. This approach was in the previous patch, but it needs a copyright clarification.

[Simon Hausmann]

I like this solution. CCing Patrick to see this has any implications for his work/port and Sam. This affects WebKit2 after all.

[Build Bot]

Comment on attachment 183779 [details]
patch moving support to xpsp2 and later

Attachment 183779 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16011351

[Csaba Osztrogonác]

Windows 2000 support period is expired at July 13, 2010. So I don't think 
if anybody want to ship any WebKit based think for an unsupported system.

[Jocelyn Turcotte]

(In reply to comment #14)
> (From update of attachment 183779 [details])
> Attachment 183779 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16011351

You might have to fix Tools/DumpRenderTree/config.h as well.

[Patrick R. Gansterer]

OS(WINCE) has no support for the 64 bit version, but since you don't touch the "#if COMPILER(MINGW) || COMPILER(MSVC7_OR_LOWER) || OS(WINCE)" area, there is no problem for WinCE, but eg. for MINGW.

[Zoltan Arvai]

You're right. I do an extended search for the 0x0500 value to find if there are any other part that has WINVER or _WIN32_WINNT values defined.

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 183779 [details] [details])
> > Attachment 183779 [details] [details] did not pass win-ews (win):
> > Output: http://queues.webkit.org/results/16011351
> 
> You might have to fix Tools/DumpRenderTree/config.h as well.

[Zoltan Arvai]

After looking in 
https://trac.webkit.org/changeset/139514/trunk/Source/WebKit2/Platform/CoreIPC/Connection.cpp 
I think WebKit2 always calls the 64bit version of atomicIncrement, so I think it could be a problem for WINCE, too :(

(In reply to comment #17)
> OS(WINCE) has no support for the 64 bit version, but since you don't touch the "#if COMPILER(MINGW) || COMPILER(MSVC7_OR_LOWER) || OS(WINCE)" area, there is no problem for WinCE, but eg. for MINGW.

[Patrick R. Gansterer]

(In reply to comment #19)
> After looking in 
> https://trac.webkit.org/changeset/139514/trunk/Source/WebKit2/Platform/CoreIPC/Connection.cpp 
> I think WebKit2 always calls the 64bit version of atomicIncrement, so I think it could be a problem for WINCE, too :(

OS(WINCE) && PLATFORM(WIN) (=my port) isn't a problem, since there is no WK2 support.
But I don't know the status of OS(WINCE) && PLATFORM(QT).

[Zoltan Arvai]

Created attachment 183794 [details]
patch for xpsp2 v2

Adding DRT's config.h modification, changing OS specific sections in Atomics.h. Testing MINGW with EWS.

[Zoltan Arvai]

Created attachment 183795 [details]
patch for xpsp2 v2b

I made a typo in the previous patch, sorry.

[Csaba Osztrogonác]

Just a note: WK2 build is still disabled on the Qt Windows buildbot
until this bug is fixed. (to be able to test at least WK1 build)

Zoltán, please don't forget to remove --no-webkit2 option after it lands. ;)

[Zoltan Arvai]

Created attachment 184205 [details]
patch for landing

No objection received in the last day on WebKit-dev list after mailing about moving support to XPSP2.

Patch updated:
- Tools/WinLauncher also updated, some style guide issue with this old code. I ignored it, because this is msvc specific header and it uses #pragma once instead of #ifndef stdafx_h.
- yesterday r140472 moved WINVER to XP (no SP) in WTF's config.h, moving to XPSP2.

[Jocelyn Turcotte]

Comment on attachment 184205 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=184205&action=review

> Source/JavaScriptCore/ChangeLog:5
> +        Dropping support for Windows versions before XP SP2
> +        for fixing atomicIncrement implementation for Windows.
> +        https://bugs.webkit.org/show_bug.cgi?id=106740

The subject line should be short enough to fit on one line.

> Source/JavaScriptCore/ChangeLog:11
> +        Current WebKit configured to support Windows 2000 and later versions.
> +        After r139514 WebKit2 can't be build, because the required InterlockedIncrement64
> +        method support is available on XP SP2 and later. So we have to move on.

The commit log editor will put all the ChangeLog bodies in the commit log. So since they are the same it would be better to omit the body except for WebKit2 (where it matters here) to avoid it appearing 6 times.

> Source/WTF/ChangeLog:11
> +        Current WebKit configured to support Windows 2000 and later versions.
> +        After r139514 WebKit2 can't be build, because the required InterlockedIncrement64
> +        method support is available on XP SP2 and later. So we have to move on.

This should rather describe the change to Atomics.h, the separation of WinCE, etc.

> Source/WTF/wtf/Atomics.h:87
> +inline int64_t atomicIncrement(int64_t* addend) { return InterlockedIncrement64(reinterpret_cast<long long* >(addend)); }
> +inline int64_t atomicDecrement(int64_t* addend) { return InterlockedDecrement64(reinterpret_cast<long long* >(addend)); }

Unneeded extra space before the last ">".

[Zoltan Arvai]

Created attachment 184545 [details]
updated patch

Modifications based on the review. I added the detailed comment to WTF's changelog, because it's affected mostly by the patch.

[WebKit Review Bot]

Attachment 184545 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/config.h', u'Source/WTF/ChangeLog', u'Source/WTF/config.h', u'Source/WTF/wtf/Atomics.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCorePrefix.h', u'Source/WebCore/config.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKitPrefix.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/config.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/config.h', u'Tools/WinLauncher/stdafx.h']" exit_code: 1
Tools/WinLauncher/stdafx.h:35:  #ifndef header guard has wrong style, please use: stdafx_h  [build/header_guard] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Comment on attachment 184545 [details]
updated patch

Looks good to me.
Could any WebKit2 owner sign this off? I can't tell if this qualifies as a "trivial fix".

[Csaba Osztrogonác]

cc wk2 owners, please review the fix as soon as possible, because Qt WK2 build is broken because this bug for 2 weeks.

[Sam Weinig]

I'm fine with the WebKit2 part.

[WebKit Review Bot]

Comment on attachment 184545 [details]
updated patch

Clearing flags on attachment: 184545

Committed r140930: <http://trac.webkit.org/changeset/140930>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.