578340 - Sync jschar and PRUnichar on Windows

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

Currently on Windows jschar is uint16 and PRUnichar is wchar_t. It's not really visible problem on MSC, as we use -Zc:wchar_t- option, which results in wchar_t being unsigned short. On mingw, however, it's not possible, so the code needs a lot of casts every time jschar/PRUnichar conversion is needed, see bug 505729 for an example. The idea is to get rid of -Zc:wchar_t- (bug 508905, soon to be landed), so the code will behave the same on both compilers (and requiring casts on MSC as well). It would be nice to avoid these casts.

As we're currently breaking API compatibility anyways, it would be nice to change jschar to wchar_t (which is 16-bit unsigned) on Windows target. The change won't affect much. The API will still be binary compatible. Apps should use jschar or, eventually, wchar_t/PRUnichar so they will still compile. The only affected apps will be ones using unsigned short for strings passed to JS, but they are worth fixing anyways.

Comment 1

[Brendan Eich [:brendan]]

If wchar_t is 32-bit unsigned on Mingw then we do not want to change jschar to alias that type.

Having to cast is a problem, but casting is covering it up. Isn't the bug that our code (since day one in mozilla.org) assumes PRUnichar and jschar are both 16-bit unsigned?

/be

Comment 2

[Brendan Eich [:brendan]]

IOW, you can't just "sync" the two typedefs. They have to agree *and* be 16-bit unsigned int types.

It might work (and waste a ton of space) to make them both 32-bit unsigned int, but I bet lots of code in JS and XPConnect counts on the chopping you get for free storing (after casting if necessary) into jschar.

/be

Comment 3

[Jacek Caban]

(In reply to comment #1)
> If wchar_t is 32-bit unsigned on Mingw then we do not want to change jschar to
> alias that type.
>
> Having to cast is a problem, but casting is covering it up. Isn't the bug that
> our code (since day one in mozilla.org) assumes PRUnichar and jschar are both
> 16-bit unsigned?

I should be clearer on this. wchar_t is 16-bit unsigned on mingw and it's binary compatible with uint16. The problem is that it's a different builtin type, so compiler requires an explicit cast here. There is the same problem with MSC, but it's hidden by -Zc:wchar_t-, that will be removed soon.

The proposed change shouldn't affect compiled binary at all.

Comment 4

[Jacek Caban]

Could you please comment if the approach will be considered for approval? I have more casts that need to be added otherwise and I don't want to file bugs for things that have a chance to be no longer needed.

Comment 5

[Brendan Eich [:brendan]]

Cc'ing C++ gurus for better ideas than adding casts till it compiles, but if that is the only way, then sure: add casts. Layering types where they are the same int type would be better, but would seem to require more ugly configury and ifdefs.

/be

Comment 6

[Luke Wagner [:luke]]

The rules for builtins generally make me sad.  One way you can avoid widespread casting (where each site is a chance for future type errors to go uncaught) is to introduce a restricted cast:

static JS_ALWAYS_INLINE jschar
PRToJSChar(PRUnichar u) { return (jschar)u; }

and then use this restricted cast everywhere.

Comment 7

[Jacek Caban]

Thanks for the reply. The main problem is not wchar_t (==PRUnichar) to jschar cast as they work implicitly, just like other integer types. The problem is pointers. I can add restricted casts like PRtoJSCharPtr and PRtoJSCharConstPtr (*Ptr names seem better than *String to not be confused with nsAString). What's the right place to put them? JS headers don't use NSPR types, so it should probably go to other header. Is there any that would be appropriate?

Also back to the original idea, is one #ifdef really enough reason to not use wchar_t for jschar in jspubtd.h?

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 457058 [details] [diff] [review]
fix v1.0

I think this patch is a fine idea. One nit: we should add
  JS_STATIC_ASSERT(WCHAR_MIN == 0);   // wchar_t is unsigned
in the WIN32 branch here.

And after the #endif, just to be totally sure,
  JS_STATIC_ASSERT(sizeof(jschar) == 2);

This way, in the unlikely case this breaks some offbeat platform or embedder, the maintainer will find out about it at compile time.

Comment 9

[Jacek Caban]

Attachment: fix with static assert

Thanks for the review. The problem with JS_STATIC_ASSERT is that it's defined in jsutils.h, that is included later than jspubtd.h, so we can't use it. In WIN32 branch, we could use C_ASSERT that is defined in winnt.h if it's included before jspubtd.h, but it seems ugly to me. Also WCHAR_MIN requires additional system includes, so I used ((jschar)-1 > 0) assert to ensure unsigned instead.

Another solution would be placing these asserts in a different header.

Comment 10

[Brendan Eich [:brendan]]

We had problems (not sure whether we still do) with static asserts in .h files. Put it in jsapi.cpp near the top, call it victory.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 459441 [details] [diff] [review]
fix with static assert

What Brendan said.

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #10)
> We had problems (not sure whether we still do) with static asserts in .h files.
> Put it in jsapi.cpp near the top, call it victory.

It's the C code that doesn't like the static asserts.  I have a few in jsval.h (#included by jspubtd.h) #ifdef'd with __cplusplus that seem to work.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #10)
> > We had problems (not sure whether we still do) with static asserts in .h files.
> > Put it in jsapi.cpp near the top, call it victory.
> 
> It's the C code that doesn't like the static asserts.  I have a few in jsval.h
> (#included by jspubtd.h) #ifdef'd with __cplusplus that seem to work.

We've also had some compiler issues with static asserts. Igor may recall.

In general putting them in .cpp files, while "less local" to the definitions they test, has dodged some porting pain.

/be

Comment 14

[Jacek Caban]

Thank you for all your comments. Pushed to TM:

http://hg.mozilla.org/tracemonkey/rev/0488f577b400

Comment 15

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/0488f577b400