119084 2013-07-25 06:07:45 -0700 [Win] Compile fix. 2013-07-25 23:54:26 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) PC Unspecified RESOLVED FIXED P2 Normal --- 0 peavo bfulgham benjamin bfulgham cmarcelo commit-queue oliver oldest_to_newest 911590 0 peavo 2013-07-25 06:07:45 -0700 While compiling JavaScriptCore, I get the following error: C:\WebKit\WebKitBuild\Release_WinCairo\include\private\wtf/Atomics.h(283): error C2059: syntax error : '' 911594 1 207453 peavo 2013-07-25 06:15:38 -0700 Created attachment 207453 Patch 911595 2 peavo 2013-07-25 06:18:37 -0700 Assembly code taken from http://stackoverflow.com/questions/5796459/is-there-an-8-bit-atomic-cas-cmpxchg-intrinsic-for-x64-in-visual-c 911739 3 207453 bfulgham 2013-07-25 13:15:24 -0700 Comment on attachment 207453 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207453&action=review I want olliej or ggaren to double-check this, but it seems fine to me. Due to Visual Studio's decision to not support inline assembly in 64-bit builds, we'll also have to start adding external ASM files on Windows builds. Maybe this code could should end up in such an external file. > Source/WTF/wtf/Atomics.h:278 > +#if OS(WINDOWS) Shouldn't this be inside the "#if ENABLE(COMPARE_AND_SWAP)" macro? I wonder if this block shouldn't be a sibling of the existing "#if CPU(X86) || CPU(X86_64)". 911744 4 bfulgham 2013-07-25 13:43:31 -0700 I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback. 911749 5 207483 peavo 2013-07-25 13:50:50 -0700 Created attachment 207483 Patch 911751 6 peavo 2013-07-25 13:53:19 -0700 (In reply to comment #4) > I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback. Ok, didn't see this comment before I updated patch, thanks :) 911754 7 bfulgham 2013-07-25 13:56:43 -0700 Landed in r153345 (http://trac.webkit.org/changeset/153345). 911755 8 bfulgham 2013-07-25 13:57:35 -0700 (In reply to comment #6) > (In reply to comment #4) > > I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback. > > Ok, didn't see this comment before I updated patch, thanks :) I changed the bug slightly to reflect that we weren't fixing the build, but were providing a nice, optimized ASM implementation. Very nice! Thanks for coming up with this fix. 911759 9 peavo 2013-07-25 14:11:37 -0700 (In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #4) > > > I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback. > > > > Ok, didn't see this comment before I updated patch, thanks :) > > I changed the bug slightly to reflect that we weren't fixing the build, but were providing a nice, optimized ASM implementation. Very nice! > > Thanks for coming up with this fix. Very good, thanks for reviewing ;) 207453 2013-07-25 06:15:38 -0700 2013-07-25 13:50:43 -0700 Patch bug-119084-20130725151526.patch text/plain 1489 peavo SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxNTMzMjIpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA3LTI1ICBwZWF2b0BvdXRsb29rLmNvbSAg PHBlYXZvQG91dGxvb2suY29tPgorCisgICAgICAgIFtXaW5dIENvbXBpbGUgZml4LgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE5MDg0CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW1wbGVtZW50ZWQgZnVu Y3Rpb24gd2Vha0NvbXBhcmVBbmRTd2FwICh1aW50OCB2ZXJzaW9uKSBmb3IgV2luZG93cy4KKwor ICAgICAgICAqIHd0Zi9BdG9taWNzLmg6CisgICAgICAgIChXVEY6OndlYWtDb21wYXJlQW5kU3dh cCk6IEltcGxlbWVudGVkIGZ1bmN0aW9uLgorCiAyMDEzLTA3LTI1ICBSeXVhbiBDaG9pICA8cnl1 YW4uY2hvaUBzYW1zdW5nLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCBidWlsZCBmaXggb24g dGhlIEVGTCBwb3J0LgpJbmRleDogU291cmNlL1dURi93dGYvQXRvbWljcy5oCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XVEYvd3RmL0F0b21pY3MuaAkocmV2aXNpb24gMTUzMzIwKQorKysgU291cmNl L1dURi93dGYvQXRvbWljcy5oCSh3b3JraW5nIGNvcHkpCkBAIC0yNzUsNiArMjc1LDIyIEBAIGlu bGluZSB2b2lkIG1lbW9yeUJhcnJpZXJCZWZvcmVVbmxvY2soKQogCiAjZW5kaWYKIAorI2lmIE9T KFdJTkRPV1MpCitpbmxpbmUgYm9vbCB3ZWFrQ29tcGFyZUFuZFN3YXAodWludDhfdCogbG9jYXRp b24sIHVpbnQ4X3QgZXhwZWN0ZWQsIHVpbnQ4X3QgbmV3VmFsdWUpCit7CisgICAgYm9vbCByZXN1 bHQgPSBmYWxzZTsKKworICAgIF9fYXNtIHsKKyAgICAgICAgbW92IGFsLCBleHBlY3RlZAorICAg ICAgICBtb3YgZWR4LCBsb2NhdGlvbgorICAgICAgICBtb3YgY2wsIG5ld1ZhbHVlCisgICAgICAg IGxvY2sgY21weGNoZyBieXRlIHB0cltlZHhdLCBjbAorICAgICAgICBzZXR6IHJlc3VsdAorICAg IH0KKworICAgIHJldHVybiByZXN1bHQ7Cit9CisjZWxzZQogaW5saW5lIGJvb2wgd2Vha0NvbXBh cmVBbmRTd2FwKHVpbnQ4X3QqIGxvY2F0aW9uLCB1aW50OF90IGV4cGVjdGVkLCB1aW50OF90IG5l d1ZhbHVlKQogewogI2lmIEVOQUJMRShDT01QQVJFX0FORF9TV0FQKQpAQCAtMzE1LDYgKzMzMSw3 IEBAIGlubGluZSBib29sIHdlYWtDb21wYXJlQW5kU3dhcCh1aW50OF90KgogICAgIHJldHVybiBm YWxzZTsKICNlbmRpZgogfQorI2VuZGlmIC8vIE9TKFdJTkRPV1MpCiAKIH0gLy8gbmFtZXNwYWNl IFdURgogCg== 207483 2013-07-25 13:50:50 -0700 2013-07-25 23:54:26 -0700 Patch bug-119084-20130725225037.patch text/plain 1406 peavo SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxNTMzMzQpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA3LTI1ICBwZWF2b0BvdXRsb29rLmNvbSAg PHBlYXZvQG91dGxvb2suY29tPgorCisgICAgICAgIFtXaW5dIENvbXBpbGUgZml4LgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE5MDg0CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW1wbGVtZW50ZWQgZnVu Y3Rpb24gd2Vha0NvbXBhcmVBbmRTd2FwICh1aW50OCB2ZXJzaW9uKSBmb3IgV2luZG93cy4KKwor ICAgICAgICAqIHd0Zi9BdG9taWNzLmg6CisgICAgICAgIChXVEY6OndlYWtDb21wYXJlQW5kU3dh cCk6IEltcGxlbWVudGVkIGZ1bmN0aW9uLgorCiAyMDEzLTA3LTI1ICBBbGxhbiBTYW5kZmVsZCBK ZW5zZW4gIDxhbGxhbi5qZW5zZW5AZGlnaWEuY29tPgogCiAgICAgICAgIEZpeCB3aW5kb3dzIGJ1 aWxkIGFmdGVyIEZUTCB1cHN0cmVhbQpJbmRleDogU291cmNlL1dURi93dGYvQXRvbWljcy5oCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3RmL0F0b21pY3MuaAkocmV2aXNpb24gMTUzMzM4KQor KysgU291cmNlL1dURi93dGYvQXRvbWljcy5oCSh3b3JraW5nIGNvcHkpCkBAIC0yNzgsNyArMjc4 LDE3IEBAIGlubGluZSB2b2lkIG1lbW9yeUJhcnJpZXJCZWZvcmVVbmxvY2soKQogaW5saW5lIGJv b2wgd2Vha0NvbXBhcmVBbmRTd2FwKHVpbnQ4X3QqIGxvY2F0aW9uLCB1aW50OF90IGV4cGVjdGVk LCB1aW50OF90IG5ld1ZhbHVlKQogewogI2lmIEVOQUJMRShDT01QQVJFX0FORF9TV0FQKQotI2lm ICFPUyhXSU5ET1dTKSAmJiAoQ1BVKFg4NikgfHwgQ1BVKFg4Nl82NCkpCisjaWYgT1MoV0lORE9X UykKKyAgICBib29sIHJlc3VsdCA9IGZhbHNlOworICAgIF9fYXNtIHsKKyAgICAgICAgbW92IGFs LCBleHBlY3RlZAorICAgICAgICBtb3YgZWR4LCBsb2NhdGlvbgorICAgICAgICBtb3YgY2wsIG5l d1ZhbHVlCisgICAgICAgIGxvY2sgY21weGNoZyBieXRlIHB0cltlZHhdLCBjbAorICAgICAgICBz ZXR6IHJlc3VsdAorICAgIH0KKyAgICByZXR1cm4gcmVzdWx0OworI2VsaWYgQ1BVKFg4NikgfHwg Q1BVKFg4Nl82NCkKICAgICB1bnNpZ25lZCBjaGFyIHJlc3VsdDsKICAgICBhc20gdm9sYXRpbGUo CiAgICAgICAgICJsb2NrOyBjbXB4Y2hnYiAlMywgJTJcblx0Igo=