41792 2010-07-07 12:59:16 -0700 [WINCE] Implement TCSpinLock 2010-07-23 09:24:57 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 paroga webkit-unassigned aroben commit-queue webkit.review.bot oldest_to_newest 247702 0 paroga 2010-07-07 12:59:16 -0700 InterlockedExchange is available on all windows platforms and can be in WinCE or as fallback: http://msdn.microsoft.com/en-us/library/ms683590%28VS.85%29.aspx 247703 1 60768 paroga 2010-07-07 13:01:27 -0700 Created attachment 60768 Patch 247704 2 webkit.review.bot 2010-07-07 13:02:17 -0700 Attachment 60768 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/TCSpinLock.h:198: "windows.h" already included at JavaScriptCore/wtf/TCSpinLock.h:55 [build/include] [4] JavaScriptCore/wtf/TCSpinLock.h:200: TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/TCSpinLock.h:205: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:210: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:214: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:215: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] JavaScriptCore/wtf/TCSpinLock.h:225: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:225: TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/TCSpinLock.h:226: One space before end of line comments [whitespace/comments] [5] Total errors found: 9 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 255388 3 60768 aroben 2010-07-23 08:25:58 -0700 Comment on attachment 60768 Patch > +#elif OS(WINDOWS) I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is: #if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC)) Will WinCE not match that condition? > + LONG m_lockword; I think m_lockword is supposed to be a volatile LONG. > +static void TCMalloc_SlowLock(LPLONG lockword) { > + Sleep(0); // Yield immediately since fast path failed > + while (true) { > + if (!InterlockedExchange(lockword, 1)) > + return; > + > + Sleep(2); > + } > +} This could be a little simpler: Sleep(0); while (InterlockedExchange(lockword, 1)) Sleep(2); 255395 4 62428 paroga 2010-07-23 08:36:40 -0700 Created attachment 62428 Patch (In reply to comment #3) > I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is: > > #if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC)) > > Will WinCE not match that condition? CPU(ARM) > > + LONG m_lockword; > > I think m_lockword is supposed to be a volatile LONG. No, otherwise you get "convert parameter 1 from 'volatile LONG *' to 'LPLONG'" 255397 5 webkit.review.bot 2010-07-23 08:38:25 -0700 Attachment 62428 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/wtf/TCSpinLock.h:198: "windows.h" already included at JavaScriptCore/wtf/TCSpinLock.h:55 [build/include] [4] JavaScriptCore/wtf/TCSpinLock.h:200: TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/TCSpinLock.h:205: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:210: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:214: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:215: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] JavaScriptCore/wtf/TCSpinLock.h:225: Place brace on its own line for function definitions. [whitespace/braces] [4] JavaScriptCore/wtf/TCSpinLock.h:225: TCMalloc_SlowLock is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] JavaScriptCore/wtf/TCSpinLock.h:226: One space before end of line comments [whitespace/comments] [5] Total errors found: 9 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 255411 6 aroben 2010-07-23 09:05:47 -0700 (In reply to comment #4) > Created an attachment (id=62428) [details] > Patch > > (In reply to comment #3) > > I don't understand how this code will be compiled for WinCE. The first branch of this #if/#elif chain is: > > > > #if (CPU(X86) || CPU(X86_64) || CPU(PPC)) && (COMPILER(GCC) || COMPILER(MSVC)) > > > > Will WinCE not match that condition? > CPU(ARM) Ah! > > > + LONG m_lockword; > > > > I think m_lockword is supposed to be a volatile LONG. > > No, otherwise you get "convert parameter 1 from 'volatile LONG *' to 'LPLONG'" I guess InterlockedExchange has a different signature on WinCE. 255419 7 62428 commit-queue 2010-07-23 09:24:52 -0700 Comment on attachment 62428 Patch Clearing flags on attachment: 62428 Committed r63981: <http://trac.webkit.org/changeset/63981> 255420 8 commit-queue 2010-07-23 09:24:57 -0700 All reviewed patches have been landed. Closing bug. 60768 2010-07-07 13:01:27 -0700 2010-07-23 08:36:40 -0700 Patch bug-41792.patch text/plain 2199 paroga SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFTY3Jp cHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gNjI2OTEpDQorKysgSmF2YVNjcmlwdENvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpDQpAQCAtMSwzICsxLDE5IEBADQorMjAxMC0wNy0wNyAgUGF0 cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FAcGFyb2dhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbV0lOQ0VdIEltcGxlbWVudCBUQ1NwaW5Mb2Nr LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDE3OTIK KworICAgICAgICBJbXBsZW1lbnQgdGhlIFNwaW5Mb2NrIHdpdGggSW50ZXJsb2NrZWRFeGNoYW5n ZSBmcm9tIHRoZSBXaW5kb3dzIEFQSS4KKworICAgICAgICAqIHd0Zi9UQ1NwaW5Mb2NrLmg6Cisg ICAgICAgIChUQ01hbGxvY19TcGluTG9jazo6TG9jayk6CisgICAgICAgIChUQ01hbGxvY19TcGlu TG9jazo6VW5sb2NrKToKKyAgICAgICAgKFRDTWFsbG9jX1NwaW5Mb2NrOjpJc0hlbGQpOgorICAg ICAgICAoVENNYWxsb2NfU3BpbkxvY2s6OkluaXQpOgorICAgICAgICAoVENNYWxsb2NfU2xvd0xv Y2spOgorCiAyMDEwLTA3LTA3ICBBbmRlcnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+ CiAKICAgICAgICAgVHJ5IHRvIGZpeCB0aGUgV2luZG93cyBidWlsZC4KSW5kZXg6IEphdmFTY3Jp cHRDb3JlL3d0Zi9UQ1NwaW5Mb2NrLmgNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBKYXZhU2NyaXB0Q29yZS93 dGYvVENTcGluTG9jay5oCShyZXZpc2lvbiA2MjU0NykNCisrKyBKYXZhU2NyaXB0Q29yZS93dGYv VENTcGluTG9jay5oCSh3b3JraW5nIGNvcHkpDQpAQCAtMSw0ICsxLDUgQEANCiAvLyBDb3B5cmln aHQgKGMpIDIwMDUsIDIwMDYsIEdvb2dsZSBJbmMuCisvLyBDb3B5cmlnaHQgKGMpIDIwMTAsIFBh dHJpY2sgR2Fuc3RlcmVyIDxwYXJvZ2FAcGFyb2dhLmNvbT4KIC8vIEFsbCByaWdodHMgcmVzZXJ2 ZWQuCiAvLyAKIC8vIFJlZGlzdHJpYnV0aW9uIGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkg Zm9ybXMsIHdpdGggb3Igd2l0aG91dApAQCAtMTg5LDYgKzE5MCw0OCBAQA0KICAgfQogfQogCisj ZWxpZiBPUyhXSU5ET1dTKQorCisjaWZuZGVmIFdJTjMyX0xFQU5fQU5EX01FQU4KKyNkZWZpbmUg V0lOMzJfTEVBTl9BTkRfTUVBTgorI2VuZGlmCisjaW5jbHVkZSA8d2luZG93cy5oPgorCitzdGF0 aWMgdm9pZCBUQ01hbGxvY19TbG93TG9jayh2b2xhdGlsZSBMUExPTkcgbG9ja3dvcmQpOworCisv LyBUaGUgZm9sbG93aW5nIGlzIGEgc3RydWN0IHNvIHRoYXQgaXQgY2FuIGJlIGluaXRpYWxpemVk IGF0IGNvbXBpbGUgdGltZQorc3RydWN0IFRDTWFsbG9jX1NwaW5Mb2NrIHsKKworICAgIGlubGlu ZSB2b2lkIExvY2soKSB7CisgICAgICAgIGlmIChJbnRlcmxvY2tlZEV4Y2hhbmdlKCZtX2xvY2t3 b3JkLCAxKSkKKyAgICAgICAgICAgIFRDTWFsbG9jX1Nsb3dMb2NrKCZtX2xvY2t3b3JkKTsKKyAg ICB9CisKKyAgICBpbmxpbmUgdm9pZCBVbmxvY2soKSB7CisgICAgICAgIEludGVybG9ja2VkRXhj aGFuZ2UoJm1fbG9ja3dvcmQsIDApOworICAgIH0KKworICAgIGlubGluZSBib29sIElzSGVsZCgp IGNvbnN0IHsKKyAgICAgICAgcmV0dXJuIG1fbG9ja3dvcmQgIT0gMDsKKyAgICB9CisKKyAgICBp bmxpbmUgdm9pZCBJbml0KCkgeyBtX2xvY2t3b3JkID0gMDsgfQorCisgICAgTE9ORyBtX2xvY2t3 b3JkOworfTsKKworI2RlZmluZSBTUElOTE9DS19JTklUSUFMSVpFUiB7IDAgfQorCitzdGF0aWMg dm9pZCBUQ01hbGxvY19TbG93TG9jayhMUExPTkcgbG9ja3dvcmQpIHsKKyAgICBTbGVlcCgwKTsg ICAgICAgIC8vIFlpZWxkIGltbWVkaWF0ZWx5IHNpbmNlIGZhc3QgcGF0aCBmYWlsZWQKKyAgICB3 aGlsZSAodHJ1ZSkgeworICAgICAgICBpZiAoIUludGVybG9ja2VkRXhjaGFuZ2UobG9ja3dvcmQs IDEpKQorICAgICAgICAgICAgcmV0dXJuOworCisgICAgICAgIFNsZWVwKDIpOworICAgIH0KK30K KwogI2Vsc2UKIAogI2luY2x1ZGUgPHB0aHJlYWQuaD4K 62428 2010-07-23 08:36:40 -0700 2010-07-23 09:24:52 -0700 Patch bug-41792.patch text/plain 2187 paroga SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFTY3Jp cHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gNjI2OTEpDQorKysgSmF2YVNjcmlwdENvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpDQpAQCAtMSwzICsxLDE5IEBADQorMjAxMC0wNy0wNyAgUGF0 cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FAcGFyb2dhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbV0lOQ0VdIEltcGxlbWVudCBUQ1NwaW5Mb2Nr LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDE3OTIK KworICAgICAgICBJbXBsZW1lbnQgdGhlIFNwaW5Mb2NrIHdpdGggSW50ZXJsb2NrZWRFeGNoYW5n ZSBmcm9tIHRoZSBXaW5kb3dzIEFQSS4KKworICAgICAgICAqIHd0Zi9UQ1NwaW5Mb2NrLmg6Cisg ICAgICAgIChUQ01hbGxvY19TcGluTG9jazo6TG9jayk6CisgICAgICAgIChUQ01hbGxvY19TcGlu TG9jazo6VW5sb2NrKToKKyAgICAgICAgKFRDTWFsbG9jX1NwaW5Mb2NrOjpJc0hlbGQpOgorICAg ICAgICAoVENNYWxsb2NfU3BpbkxvY2s6OkluaXQpOgorICAgICAgICAoVENNYWxsb2NfU2xvd0xv Y2spOgorCiAyMDEwLTA3LTA3ICBBbmRlcnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+ CiAKICAgICAgICAgVHJ5IHRvIGZpeCB0aGUgV2luZG93cyBidWlsZC4KSW5kZXg6IEphdmFTY3Jp cHRDb3JlL3d0Zi9UQ1NwaW5Mb2NrLmgNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBKYXZhU2NyaXB0Q29yZS93 dGYvVENTcGluTG9jay5oCShyZXZpc2lvbiA2MjU0NykNCisrKyBKYXZhU2NyaXB0Q29yZS93dGYv VENTcGluTG9jay5oCSh3b3JraW5nIGNvcHkpDQpAQCAtMSw0ICsxLDUgQEANCiAvLyBDb3B5cmln aHQgKGMpIDIwMDUsIDIwMDYsIEdvb2dsZSBJbmMuDQorLy8gQ29weXJpZ2h0IChjKSAyMDEwLCBQ YXRyaWNrIEdhbnN0ZXJlciA8cGFyb2dhQHBhcm9nYS5jb20+DQogLy8gQWxsIHJpZ2h0cyByZXNl cnZlZC4NCiAvLyANCiAvLyBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQgYmlu YXJ5IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQNCkBAIC0xODksNiArMTkwLDQ0IEBADQogICB9DQog fQ0KIA0KKyNlbGlmIE9TKFdJTkRPV1MpDQorDQorI2lmbmRlZiBXSU4zMl9MRUFOX0FORF9NRUFO DQorI2RlZmluZSBXSU4zMl9MRUFOX0FORF9NRUFODQorI2VuZGlmDQorI2luY2x1ZGUgPHdpbmRv d3MuaD4NCisNCitzdGF0aWMgdm9pZCBUQ01hbGxvY19TbG93TG9jayhMUExPTkcgbG9ja3dvcmQp Ow0KKw0KKy8vIFRoZSBmb2xsb3dpbmcgaXMgYSBzdHJ1Y3Qgc28gdGhhdCBpdCBjYW4gYmUgaW5p dGlhbGl6ZWQgYXQgY29tcGlsZSB0aW1lDQorc3RydWN0IFRDTWFsbG9jX1NwaW5Mb2NrIHsNCisN CisgICAgaW5saW5lIHZvaWQgTG9jaygpIHsNCisgICAgICAgIGlmIChJbnRlcmxvY2tlZEV4Y2hh bmdlKCZtX2xvY2t3b3JkLCAxKSkNCisgICAgICAgICAgICBUQ01hbGxvY19TbG93TG9jaygmbV9s b2Nrd29yZCk7DQorICAgIH0NCisNCisgICAgaW5saW5lIHZvaWQgVW5sb2NrKCkgew0KKyAgICAg ICAgSW50ZXJsb2NrZWRFeGNoYW5nZSgmbV9sb2Nrd29yZCwgMCk7DQorICAgIH0NCisNCisgICAg aW5saW5lIGJvb2wgSXNIZWxkKCkgY29uc3Qgew0KKyAgICAgICAgcmV0dXJuIG1fbG9ja3dvcmQg IT0gMDsNCisgICAgfQ0KKw0KKyAgICBpbmxpbmUgdm9pZCBJbml0KCkgeyBtX2xvY2t3b3JkID0g MDsgfQ0KKw0KKyAgICBMT05HIG1fbG9ja3dvcmQ7DQorfTsNCisNCisjZGVmaW5lIFNQSU5MT0NL X0lOSVRJQUxJWkVSIHsgMCB9DQorDQorc3RhdGljIHZvaWQgVENNYWxsb2NfU2xvd0xvY2soTFBM T05HIGxvY2t3b3JkKSB7DQorICAgIFNsZWVwKDApOyAgICAgICAgLy8gWWllbGQgaW1tZWRpYXRl bHkgc2luY2UgZmFzdCBwYXRoIGZhaWxlZA0KKyAgICB3aGlsZSAoSW50ZXJsb2NrZWRFeGNoYW5n ZShsb2Nrd29yZCwgMSkpDQorICAgICAgICBTbGVlcCgyKTsNCit9DQorDQogI2Vsc2UNCiANCiAj aW5jbHVkZSA8cHRocmVhZC5oPg0K