| Summary: | Fix the atomicIncrement implementation for Windows | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||||||
| Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | aestes, andersca, ap, bdakin, beidson, benjamin, darin, enrica, hausmann, hyatt, jturcotte, kling, mitz, mjs, mrowe, ojan.autocc, ossy, paroga, pierre.rossi, sam, simon.fraser, syoichi, thorton, webkit.review.bot, zarvai | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 420+ | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 106708, 106729 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Csaba Osztrogonác
2013-01-13 01:25:48 PST
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. 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? ping? 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. 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
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;
}
QtTestBrowser is absolutely irrelevant, because 64-bit atomicIncrement is only used by WebKit2. Please try it with MiniBrowser without rdesktop. (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. (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 :) Created attachment 183457 [details]
patch
let see ews results...
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.
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.
I like this solution. CCing Patrick to see this has any implications for his work/port and Sam. This affects WebKit2 after all. 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 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. (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. 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. 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. 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. (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). 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.
Created attachment 183795 [details]
patch for xpsp2 v2b
I made a typo in the previous patch, sorry.
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. ;) 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. 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 ">". 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.
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.
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".
cc wk2 owners, please review the fix as soon as possible, because Qt WK2 build is broken because this bug for 2 weeks. I'm fine with the WebKit2 part. Comment on attachment 184545 [details] updated patch Clearing flags on attachment: 184545 Committed r140930: <http://trac.webkit.org/changeset/140930> All reviewed patches have been landed. Closing bug. |