34560 2010-02-03 21:45:40 -0800 Typedef both JSChar and UChar to wchar_t in RVCT. 2010-02-17 12:24:42 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Other RESOLVED FIXED P2 Normal --- 33564 0 skyul webkit-unassigned commit-queue dbates eric gustavo hausmann joybro201 koshuin laszlo.gombos webkit.review.bot xan.lopez oldest_to_newest 187630 0 skyul 2010-02-03 21:45:40 -0800 Both the defined type wchar_t in C and the native type wchar_t in C++ are unsigned short with --wchar16 option. RVCT assumes --wchar16 unless --wchar32 is explicitly specified, so wchar_t is compatible with JSChar. 187631 1 48102 skyul 2010-02-03 21:48:07 -0800 Created attachment 48102 Patch 187646 2 webkit.review.bot 2010-02-03 23:31:43 -0800 Attachment 48102 did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/236020 187698 3 hausmann 2010-02-04 02:33:34 -0800 This doesn't look correct to me. Shouldn't you pass that option to RVCT then? We don't see this issue when compiling with WebKit for ARM with RVCT 2.2 for Symbian. In fact I think your patch is going to break the build for that combination. 187716 4 skyul 2010-02-04 03:23:44 -0800 (In reply to comment #3) > This doesn't look correct to me. Shouldn't you pass that option to RVCT then? > > We don't see this issue when compiling with WebKit for ARM with RVCT 2.2 for > Symbian. In fact I think your patch is going to break the build for that > combination. That's weird. When I compiled WebKit BREW port for ARM with RVCT 2.2 and I had to apply this patch. I will check this again and let you know. 187717 5 48102 skyul 2010-02-04 03:24:39 -0800 Comment on attachment 48102 Patch Cancel the review request as it breaks the gtk build anyway. 188961 6 skyul 2010-02-09 04:15:58 -0800 Without this patch, I encountered the following RVCT compile error: [ 505/1814] cxx: JavaScriptCore\API\JSStringRef.cpp -> build\default\JavaScriptCore\API\JSStringRef_1.o "..\JavaScriptCore\API\JSStringRef.cpp", line 39: Error: #304: no instance of overloaded function "OpaqueJSString::create" matches the argument list argument types are: (const JSChar *, std::size_t) return OpaqueJSString::create(chars, numChars).releaseRef(); ^ "..\JavaScriptCore\API\JSStringRef.cpp", line 75: Error: #120: return value type does not match the function type return string->characters(); ^ ..\JavaScriptCore\API\JSStringRef.cpp: 0 warnings, 2 errors Waf: Leaving directory `C:\cygwin\home\skyul\WebKit\build' Build failed -> task failed (err #1): {task: cxx JSStringRef.cpp -> JSStringRef_1.o} This complains about the incompatibility between JSChar* and UChar*. UChar is defined in ICU's umachine.h /* Define UChar to be compatible with wchar_t if possible. */ #if U_SIZEOF_WCHAR_T==2 typedef wchar_t UChar; #else typedef uint16_t UChar; #endif As U_SIZEOF_WCHAR_T==2, UChar is wchar_t. JSChar is defined in JSStringRef.h #if !defined(WIN32) && !defined(_WIN32) && !defined(__WINSCW__) /*! @typedef JSChar @abstract A Unicode character. */ typedef unsigned short JSChar; #else typedef wchar_t JSChar; #endif JSChar is defined to unsigned short. This patch is needed to make JSChar* and UChar* compatible by defining both types to wchar_t. I am not sure why Symbian Qt port does not have this problem. Maybe they define UChar to unsigned short? 188963 7 skyul 2010-02-09 04:29:36 -0800 I did a little experiment with the following code snippet: #include <wchar.h> typedef unsigned short JSChar; typedef wchar_t UChar; void foo(UChar* u) { } void bar() { JSChar* j; foo(j); } RVCT compiles this code without an error in C mode. However, it complains about the type compatibility in C++ mode. "a.cpp", line 13: Error: #167: argument of type "JSChar *" is incompatible with parameter of type "UChar *" foo(j); ^ a.cpp: 0 warnings, 1 error wchar_t in C++ is not a simple typedef of short int, it is a unique type with size of 2. So we need to typedef both JSChar and UChar to wchar_t for compatibility. 188979 8 koshuin 2010-02-09 06:17:10 -0800 (In reply to comment #7) > the type compatibility in C++ mode. > > "a.cpp", line 13: Error: #167: argument of type "JSChar *" is incompatible > with parameter of type "UChar *" > foo(j); > ^ > a.cpp: 0 warnings, 1 error > > > wchar_t in C++ is not a simple typedef of short int, it is a unique type with > size of 2. So we need to typedef both JSChar and UChar to wchar_t for > compatibility. I find this piece odd. I ran your example with --cpp and --c90 and observed the same results. however if you look at the changes made into webkit from Symbian port you can see we don't define this and it works :S Note that with Qt in JavaScriptCore/wtf/unicode/UnicodeQt4.h we have: // ugly hack to make UChar compatible with JSChar in API/JSStringRef.h #if defined(Q_OS_WIN) || COMPILER(WINSCW) typedef wchar_t UChar; #else typedef uint16_t UChar; That part is propably missing from your patch i.e. use the 'ugly hack' for RVCT as well. -Janne 188984 9 koshuin 2010-02-09 06:45:50 -0800 So the reason why it works for Symbian port is indeed this line: > typedef uint16_t UChar; So if the patch is to be applied both files UnicodeQt4.h JSStringRef.h need to be changed. 189024 10 skyul 2010-02-09 08:11:02 -0800 (In reply to comment #9) > So the reason why it works for Symbian port is indeed this line: > > typedef uint16_t UChar; > > So if the patch is to be applied both files > UnicodeQt4.h > JSStringRef.h > need to be changed. It makes sense. I will update the patch. 189046 11 48423 skyul 2010-02-09 09:24:17 -0800 Created attachment 48423 Patch Typedef both JSChar and UChar to wchar_t in RVCT. UnicodeQt4.h is modified to define UChar to wchar_t for RVCT. Don't use COMPILER(RVCT) guard in JavaScriptCore/API/JSStringRef.h as it breaks the gtk port. Use defined(__CC_ARM) || defined(__ARMCC__) instead. 189048 12 webkit.review.bot 2010-02-09 09:31:50 -0800 Attachment 48423 did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/250591 189051 13 48424 skyul 2010-02-09 09:35:11 -0800 Created attachment 48424 Patch Oops. The gtk port fails to build due to C++ style comment (//). Replace it with C style comment (/* */). 189114 14 48424 abarth 2010-02-09 12:12:14 -0800 Comment on attachment 48424 Patch I'm not an expert here, but it looks like you've incorporated the feedback you've received and we already have similar ifdefs in those locations. 189178 15 skyul 2010-02-09 17:05:28 -0800 (In reply to comment #14) > (From update of attachment 48424 [details]) > I'm not an expert here, but it looks like you've incorporated the feedback > you've received and we already have similar ifdefs in those locations. Would you set commit-queue+ too? Because I am not a committer, I can't commit it by myself. 189835 16 48424 commit-queue 2010-02-11 16:52:39 -0800 Comment on attachment 48424 Patch Rejecting patch 48424 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12147 test cases. storage/close-during-stress-test.html -> crashed Exiting early after 1 failures. 9165 tests run. 328.13s total testing time 9164 test cases (99%) succeeded 1 test case (<1%) crashed 6 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/259352 189866 17 skyul 2010-02-11 18:39:02 -0800 (In reply to comment #16) > (From update of attachment 48424 [details]) > Rejecting patch 48424 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', > '--exit-after-n-failures=1', '--quiet']" exit_code: 1 > Running build-dumprendertree > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Testing 12147 test cases. > storage/close-during-stress-test.html -> crashed > > Exiting early after 1 failures. 9165 tests run. > 328.13s total testing time > > 9164 test cases (99%) succeeded > 1 test case (<1%) crashed > 6 test cases (<1%) had stderr output > > Full output: http://webkit-commit-queue.appspot.com/results/259352 It seems the crash is not related to this patch as this patch only affects RVCT compiler. 189987 18 48424 commit-queue 2010-02-12 02:20:29 -0800 Comment on attachment 48424 Patch Clearing flags on attachment: 48424 Committed r54717: <http://trac.webkit.org/changeset/54717> 189988 19 commit-queue 2010-02-12 02:20:37 -0800 All reviewed patches have been landed. Closing bug. 191146 20 eric 2010-02-17 12:24:42 -0800 The crash was bug 30172. 48102 2010-02-03 21:48:07 -0800 2010-02-09 09:24:17 -0800 Patch wchar_t.patch text/plain 1222 skyul SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDU0MzIyKQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDItMDMgIEt3YW5nIFl1 bCBTZW8gIDxza3l1bEBjb21wYW55MTAwLm5ldD4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUeXBlZGVmIEpTQ2hhciB0byB3Y2hhcl90IGluIFJWQ1Qu CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNDU2MAor CisgICAgICAgIEJvdGggdGhlIGRlZmluZWQgdHlwZSB3Y2hhcl90IGluIEMgYW5kIHRoZSBuYXRp dmUgdHlwZSB3Y2hhcl90IGluIEMrKyBhcmUKKyAgICAgICAgdW5zaWduZWQgc2hvcnQgd2l0aCAt LXdjaGFyMTYgb3B0aW9uLiBSVkNUIGFzc3VtZXMgLS13Y2hhcjE2IHVubGVzcyAtLXdjaGFyMzIK KyAgICAgICAgaXMgZXhwbGljaXRseSBzcGVjaWZpZWQsIHNvIHdjaGFyX3QgaXMgY29tcGF0aWJs ZSB3aXRoIEpTQ2hhci4KKworICAgICAgICAqIEFQSS9KU1N0cmluZ1JlZi5oOgorCiAyMDEwLTAy LTAzICBLd2FuZyBZdWwgU2VvICA8c2t5dWxAY29tcGFueTEwMC5uZXQ+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgRXJpYyBTZWlkZWwuCkluZGV4OiBKYXZhU2NyaXB0Q29yZS9BUEkvSlNTdHJpbmdS ZWYuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0Q29yZS9BUEkvSlNTdHJpbmdSZWYuaAkocmV2 aXNpb24gNTQzMjIpCisrKyBKYXZhU2NyaXB0Q29yZS9BUEkvSlNTdHJpbmdSZWYuaAkod29ya2lu ZyBjb3B5KQpAQCAtMzcsNyArMzcsNyBAQAogZXh0ZXJuICJDIiB7CiAjZW5kaWYKIAotI2lmICFk ZWZpbmVkKFdJTjMyKSAmJiAhZGVmaW5lZChfV0lOMzIpICYmICFkZWZpbmVkKF9fV0lOU0NXX18p CisjaWYgIWRlZmluZWQoV0lOMzIpICYmICFkZWZpbmVkKF9XSU4zMikgJiYgIWRlZmluZWQoX19X SU5TQ1dfXykgJiYgIUNPTVBJTEVSKFJWQ1QpCiAvKiEKIEB0eXBlZGVmIEpTQ2hhcgogQGFic3Ry YWN0IEEgVW5pY29kZSBjaGFyYWN0ZXIuCg== 48423 2010-02-09 09:24:17 -0800 2010-02-09 09:35:11 -0800 Patch wchar_t.patch text/plain 1783 skyul SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDU0NTU2KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDItMDkgIEt3YW5nIFl1 bCBTZW8gIDxza3l1bEBjb21wYW55MTAwLm5ldD4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUeXBlZGVmIGJvdGggSlNDaGFyIGFuZCBVQ2hhciB0byB3 Y2hhcl90IGluIFJWQ1QuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0zNDU2MAorCisgICAgICAgIERlZmluZSBib3RoIEpTQ2hhciBhbmQgVUNoYXIgdG8g d2NoYXJfdCBhcyB0aGUgc2l6ZQorICAgICAgICBvZiB3Y2hhcl90IGlzIDIgYnl0ZXMgaW4gUlZD VC4KKworICAgICAgICAqIEFQSS9KU1N0cmluZ1JlZi5oOgorICAgICAgICAqIHd0Zi91bmljb2Rl L3F0NC9Vbmljb2RlUXQ0Lmg6CisKIDIwMTAtMDItMDkgIE5pa29sYXMgWmltbWVybWFubiAgPG56 aW1tZXJtYW5uQHJpbS5jb20+CiAKICAgICAgICAgTm90IHJldmlld2VkLiBUcnkgdG8gZml4IGJ1 aWxkIHByb2JsZW0gb24gU25vd0xlb3BhcmQgc2xhdmVzIHRvIGJyaW5nIHRoZW0gYmFjay4KSW5k ZXg6IEphdmFTY3JpcHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFT Y3JpcHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCShyZXZpc2lvbiA1NDU1NikKKysrIEphdmFTY3Jp cHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCSh3b3JraW5nIGNvcHkpCkBAIC0zNyw3ICszNyw4IEBA CiBleHRlcm4gIkMiIHsKICNlbmRpZgogCi0jaWYgIWRlZmluZWQoV0lOMzIpICYmICFkZWZpbmVk KF9XSU4zMikgJiYgIWRlZmluZWQoX19XSU5TQ1dfXykKKyNpZiAhZGVmaW5lZChXSU4zMikgJiYg IWRlZmluZWQoX1dJTjMyKSAmJiAhZGVmaW5lZChfX1dJTlNDV19fKSBcCisgICAgJiYgIShkZWZp bmVkKF9fQ0NfQVJNKSB8fCBkZWZpbmVkKF9fQVJNQ0NfXykpIC8vIFJWQ1QKIC8qIQogQHR5cGVk ZWYgSlNDaGFyCiBAYWJzdHJhY3QgQSBVbmljb2RlIGNoYXJhY3Rlci4KSW5kZXg6IEphdmFTY3Jp cHRDb3JlL3d0Zi91bmljb2RlL3F0NC9Vbmljb2RlUXQ0LmgKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2YVNj cmlwdENvcmUvd3RmL3VuaWNvZGUvcXQ0L1VuaWNvZGVRdDQuaAkocmV2aXNpb24gNTQ1NTYpCisr KyBKYXZhU2NyaXB0Q29yZS93dGYvdW5pY29kZS9xdDQvVW5pY29kZVF0NC5oCSh3b3JraW5nIGNv cHkpCkBAIC01Niw3ICs1Niw3IEBAIG5hbWVzcGFjZSBRVW5pY29kZVRhYmxlcyB7CiBRVF9FTkRf TkFNRVNQQUNFCiAKIC8vIHVnbHkgaGFjayB0byBtYWtlIFVDaGFyIGNvbXBhdGlibGUgd2l0aCBK U0NoYXIgaW4gQVBJL0pTU3RyaW5nUmVmLmgKLSNpZiBkZWZpbmVkKFFfT1NfV0lOKSB8fCBDT01Q SUxFUihXSU5TQ1cpCisjaWYgZGVmaW5lZChRX09TX1dJTikgfHwgQ09NUElMRVIoV0lOU0NXKSB8 fCBDT01QSUxFUihSVkNUKQogdHlwZWRlZiB3Y2hhcl90IFVDaGFyOwogI2Vsc2UKIHR5cGVkZWYg dWludDE2X3QgVUNoYXI7Cg== 48424 2010-02-09 09:35:11 -0800 2010-02-12 02:20:28 -0800 Patch wchar_t.patch text/plain 1786 skyul SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDU0NTU2KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDItMDkgIEt3YW5nIFl1 bCBTZW8gIDxza3l1bEBjb21wYW55MTAwLm5ldD4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUeXBlZGVmIGJvdGggSlNDaGFyIGFuZCBVQ2hhciB0byB3 Y2hhcl90IGluIFJWQ1QuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0zNDU2MAorCisgICAgICAgIERlZmluZSBib3RoIEpTQ2hhciBhbmQgVUNoYXIgdG8g d2NoYXJfdCBhcyB0aGUgc2l6ZQorICAgICAgICBvZiB3Y2hhcl90IGlzIDIgYnl0ZXMgaW4gUlZD VC4KKworICAgICAgICAqIEFQSS9KU1N0cmluZ1JlZi5oOgorICAgICAgICAqIHd0Zi91bmljb2Rl L3F0NC9Vbmljb2RlUXQ0Lmg6CisKIDIwMTAtMDItMDkgIE5pa29sYXMgWmltbWVybWFubiAgPG56 aW1tZXJtYW5uQHJpbS5jb20+CiAKICAgICAgICAgTm90IHJldmlld2VkLiBUcnkgdG8gZml4IGJ1 aWxkIHByb2JsZW0gb24gU25vd0xlb3BhcmQgc2xhdmVzIHRvIGJyaW5nIHRoZW0gYmFjay4KSW5k ZXg6IEphdmFTY3JpcHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFT Y3JpcHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCShyZXZpc2lvbiA1NDU1NikKKysrIEphdmFTY3Jp cHRDb3JlL0FQSS9KU1N0cmluZ1JlZi5oCSh3b3JraW5nIGNvcHkpCkBAIC0zNyw3ICszNyw4IEBA CiBleHRlcm4gIkMiIHsKICNlbmRpZgogCi0jaWYgIWRlZmluZWQoV0lOMzIpICYmICFkZWZpbmVk KF9XSU4zMikgJiYgIWRlZmluZWQoX19XSU5TQ1dfXykKKyNpZiAhZGVmaW5lZChXSU4zMikgJiYg IWRlZmluZWQoX1dJTjMyKSAmJiAhZGVmaW5lZChfX1dJTlNDV19fKSBcCisgICAgJiYgIShkZWZp bmVkKF9fQ0NfQVJNKSB8fCBkZWZpbmVkKF9fQVJNQ0NfXykpIC8qIFJWQ1QgKi8KIC8qIQogQHR5 cGVkZWYgSlNDaGFyCiBAYWJzdHJhY3QgQSBVbmljb2RlIGNoYXJhY3Rlci4KSW5kZXg6IEphdmFT Y3JpcHRDb3JlL3d0Zi91bmljb2RlL3F0NC9Vbmljb2RlUXQ0LmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2 YVNjcmlwdENvcmUvd3RmL3VuaWNvZGUvcXQ0L1VuaWNvZGVRdDQuaAkocmV2aXNpb24gNTQ1NTYp CisrKyBKYXZhU2NyaXB0Q29yZS93dGYvdW5pY29kZS9xdDQvVW5pY29kZVF0NC5oCSh3b3JraW5n IGNvcHkpCkBAIC01Niw3ICs1Niw3IEBAIG5hbWVzcGFjZSBRVW5pY29kZVRhYmxlcyB7CiBRVF9F TkRfTkFNRVNQQUNFCiAKIC8vIHVnbHkgaGFjayB0byBtYWtlIFVDaGFyIGNvbXBhdGlibGUgd2l0 aCBKU0NoYXIgaW4gQVBJL0pTU3RyaW5nUmVmLmgKLSNpZiBkZWZpbmVkKFFfT1NfV0lOKSB8fCBD T01QSUxFUihXSU5TQ1cpCisjaWYgZGVmaW5lZChRX09TX1dJTikgfHwgQ09NUElMRVIoV0lOU0NX KSB8fCBDT01QSUxFUihSVkNUKQogdHlwZWRlZiB3Y2hhcl90IFVDaGFyOwogI2Vsc2UKIHR5cGVk ZWYgdWludDE2X3QgVUNoYXI7Cg==