Bug 34560

Created attachment 48102 [details] Patch

Attachment 48102 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/236020

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.

(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.

Comment on attachment 48102 [details] Patch Cancel the review request as it breaks the gtk build anyway.

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?

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.

(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

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.

(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.

Created attachment 48423 [details] 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.

Attachment 48423 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/250591

Created attachment 48424 [details] Patch Oops. The gtk port fails to build due to C++ style comment (//). Replace it with C style comment (/* */).

Comment on attachment 48424 [details] 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.

(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.

Comment on attachment 48424 [details] 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

(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.

Comment on attachment 48424 [details] Patch Clearing flags on attachment: 48424 Committed r54717: <http://trac.webkit.org/changeset/54717>

All reviewed patches have been landed. Closing bug.

The crash was bug 30172.