WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
34560
Typedef both JSChar and UChar to wchar_t in RVCT.
https://bugs.webkit.org/show_bug.cgi?id=34560
Summary
Typedef both JSChar and UChar to wchar_t in RVCT.
Kwang Yul Seo
Reported
2010-02-03 21:45:40 PST
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.
Attachments
Patch
(1.19 KB, patch)
2010-02-03 21:48 PST
,
Kwang Yul Seo
no flags
Details
Formatted Diff
Diff
Patch
(1.74 KB, patch)
2010-02-09 09:24 PST
,
Kwang Yul Seo
no flags
Details
Formatted Diff
Diff
Patch
(1.74 KB, patch)
2010-02-09 09:35 PST
,
Kwang Yul Seo
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Kwang Yul Seo
Comment 1
2010-02-03 21:48:07 PST
Created
attachment 48102
[details]
Patch
WebKit Review Bot
Comment 2
2010-02-03 23:31:43 PST
Attachment 48102
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/236020
Simon Hausmann
Comment 3
2010-02-04 02:33:34 PST
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.
Kwang Yul Seo
Comment 4
2010-02-04 03:23:44 PST
(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.
Kwang Yul Seo
Comment 5
2010-02-04 03:24:39 PST
Comment on
attachment 48102
[details]
Patch Cancel the review request as it breaks the gtk build anyway.
Kwang Yul Seo
Comment 6
2010-02-09 04:15:58 PST
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?
Kwang Yul Seo
Comment 7
2010-02-09 04:29:36 PST
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.
Janne Koskinen
Comment 8
2010-02-09 06:17:10 PST
(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
Janne Koskinen
Comment 9
2010-02-09 06:45:50 PST
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.
Kwang Yul Seo
Comment 10
2010-02-09 08:11:02 PST
(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.
Kwang Yul Seo
Comment 11
2010-02-09 09:24:17 PST
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.
WebKit Review Bot
Comment 12
2010-02-09 09:31:50 PST
Attachment 48423
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/250591
Kwang Yul Seo
Comment 13
2010-02-09 09:35:11 PST
Created
attachment 48424
[details]
Patch Oops. The gtk port fails to build due to C++ style comment (//). Replace it with C style comment (/* */).
Adam Barth
Comment 14
2010-02-09 12:12:14 PST
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.
Kwang Yul Seo
Comment 15
2010-02-09 17:05:28 PST
(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.
WebKit Commit Bot
Comment 16
2010-02-11 16:52:39 PST
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
Kwang Yul Seo
Comment 17
2010-02-11 18:39:02 PST
(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.
WebKit Commit Bot
Comment 18
2010-02-12 02:20:29 PST
Comment on
attachment 48424
[details]
Patch Clearing flags on attachment: 48424 Committed
r54717
: <
http://trac.webkit.org/changeset/54717
>
WebKit Commit Bot
Comment 19
2010-02-12 02:20:37 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 20
2010-02-17 12:24:42 PST
The crash was
bug 30172
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug