Bug 34560 – Typedef both JSChar and UChar to wchar_t in RVCT.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

Created attachment 48102 [details]
Patch

[WebKit Review Bot]

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

[Simon Hausmann]

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]

(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 on attachment 48102 [details]
Patch

Cancel the review request as it breaks the gtk build anyway.

[Kwang Yul Seo]

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]

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]

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

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]

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

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]

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

[Kwang Yul Seo]

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 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]

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

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

Clearing flags on attachment: 48424

Committed r54717: <http://trac.webkit.org/changeset/54717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

The crash was bug 30172.