280942 - MinGW build fails on xpcstring.cpp

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Daniel Kraft]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050129
Build Identifier: 

I tried to compile the current cvs checkout, but the build failed at
js/src/xpconnect/src/xpcstring.cpp because of an invalid conversion from
PRUnichar* to jschar* and vice versa later on.
With a NS_REINTERPRET_CAST it compiles (and this cast is already used in that file).

Reproducible: Always

Steps to Reproduce:

Comment 1

[Daniel Kraft]

Attachment: patch

Like that. I didn't test it excessive, but in general JS strings still work...

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

note bug 235499 comment 38

Comment 3

[David G King]

Caused by Bug #235499

Comment 4

[David G King]

This patch fixes my gcc build problems.

Comment 5

[Daniel Kraft]

With the reinterpret cast applied, I don't see any problems - but of course,
this is a rather aggressive method...
How can I test if xpcstring still works correctly?

Comment 6

[David Bradley]

Does the compiler give any more diagnostics than just that the conversion
failed? It would be interesting to know what it thinks PRUnichar and jschar are.
What version of gcc are you using?

Tracing back the types it looks like they should be both unsigned short's.

PRUnichar looks to be defined here:
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#305

jschar is defined as uint16, which is JSUint16 which is defined here:
http://lxr.mozilla.org/seamonkey/source/js/src/jstypes.h#269

Looks like a compiler quirk to me. The cast may be the only way to get around
this. If accepted I would add comments noting why the cast is being done,
because it's sure to peek someone's curiousity when they visit this code.

Comment 7

[Daniel Kraft]

It's 3.3.3
No, I don't think the compiler gives any additional information. As there aren't
any problems using this cast, I suppose these types are also in this case the
same. Why the compiler needs the cast isn't clear to me, but maybe it thinks of
PRUint16 and jschar as different types instead of resolving them for comparison?

Comment 8

[David G King]

I'm using gcc 3.4.2, and as with Daniel, no other info is given (when the patch
isn't applied).

Comment 9

[David Bradley]

The latest gcc release is 3.4.3. I wonder if this problem exists in that version?

Shaver, what's the past history for this type of thing. Do we muck up our code
to support various errant compiler versions? Or just require people to upgrade
their compiler?

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 173299 [details] [diff] [review]
patch

I actually thought that C++ distinguished between typedef'd types in some
cases, but I'm way too tired to go to D&E or my reference.  We need to take
this patch, I think -- where it doesn't have a negative effect on performance,
we should support as many build environments as possible, IMO.

Comment 11

[David Bradley]

Comment on attachment 173299 [details] [diff] [review]
patch

r=dbradley

I would have expected VC++ .Net 2003 to complain as it's pretty close in
standard compliance, but maybe it's still lagging behind in that area.

In any case, I'd add a comment just noting why the reinterpret cast is there as
it's probably going to peek someone's interest when they see it.

Comment 12

[Daniel Kraft]

Attachment: including comment

The same with explanatory comment.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Note that in PRUnichar is _also_ defined in nscore.h which is guarded by
autoconf defines coming from configure.in (differently from the way it's defined
in prtypes.h).  The PRUnichar definition in nscore.h is the one that's being
used by xpcstring.cpp, according to "make xpcstring.i".

Comment 14

[David Bradley]

There's a piece to the puzzle missing. I tried calling a function that took a
wchar_t pointer with a unsigned int pointer and VC++ 2003 generated an error. I
couldn't figure out a compiler option that would allow that to pass. So I'm not
sure how it's even working in VC++.

In any case, I'm wondering if the better solution would be to provide a
conversion between the two types.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Are we actually confident that the low-level problem is just a compiler quirk? 
The experiments I did made it look like that it's at least a possibility that
the autoconf test is somehow broken on MinGW.

Comment 16

[David Bradley]

Yes, I'm no longer certain that this is a compiler quirk. What I haven't figure
out is what compiler option is making it work on VC++ 2003, as if I take a
simple test case it fails there as well. I've turned on and off language
extensions, and treating wchar_t as a native type.

Comment 17

[Adam]

(In reply to comment #12)
> Created an attachment (id=173665) [edit]
> including comment
> 
> The same with explanatory comment.

That fixed the same problem I was having with VC8 beta 1.

Compiling fine here now.

Comment 18

[Daniel Kraft]

As both types are the same (unsigned short), a reinterpret cast is the best
method to me. Isn't it?

Comment 19

[David Bradley]

It's not if this is due to an incorrect build setting or autoconf test. Need 
someone with build knowledge to jump in and check it out.

Comment 20

[David Bradley]

Looking for someone with autoconf/build knowledge in this area.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

I did some more testing over the weekend, and I think David's original analysis
is probably correct.  I'll post more here later this afternoon.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: minimal test case

Here's a program which is essentially a minimized test case that I created by
working backwords from xpcstring.i.  Running this program produces the
following output:

sizeof(jschar) = 2
(jschar)-1 = 65535
jschar is unsigned

sizeof(PRUnichar) = 2
(PRUnichar)-1 = 65535
PRUnichar is unsigned

Setting the #define always causes a compilation error:

a.cpp: In function `int main()':
a.cpp:20: error: invalid conversion from `jschar*' to `PRUnichar*'
a.cpp:21: error: invalid conversion from `PRUnichar*' to `jschar*'

Both the output and error conditions seem to be unaffected by -fshort-wchar,
-fno-short-wchar.

Comment 23

[MORITA Hajime]

Hello,

New C++ compilers including gcc-3.4.x have wchar_t as its built-in type, 
and distinguish integral types like unsinged short and 
character types like wchar_t.
So this bug is not their quirks. It is standard aligned behaviour.

In other hand, older version compilers do typedef wchar_t with integral type
in some header files.
For example, MSVC++ define like following in WCTYPE.H and WCHAR.H.
My MinGW-3.2.0-rc-3 also does similar in wctype.h

 #ifndef _WCHAR_T_DEFINED
 typedef unsigned short wchar_t;
 #define _WCHAR_T_DEFINED
 #endif

This (unexpectedly) allows to current xpcstring.cpp be compilable.

Possible workaronds I can imagine are:
1. use NS_REINTERPRET_CAST (use Daniel's patch)
1. define PRUnichar as PRUint16.
3. or, put the typedef above in somewhere.

I prefer 1. That has the minimum impact. 
Because similar problem can happen in the future, we need some documentations
to notice the pitfall to the developers.
2 and 3 seems hard. because PRUnichar is defined multiple files and
their conditions are slightly differenct each other.

Comment 24

[Eli Friedman]

Attachment: Simpler patch

Why not just do something like this?  Data for nsStringBuffer is of type void*,
so it makes no sense to cast to PRUnichar* when going between an nsStringBuffer
and a JSString.

Comment 25

[Daniel Kraft]

Looks good to me. This should really be better, I think.

Comment 26

[David Bradley]

Comment on attachment 176415 [details] [diff] [review]
Simpler patch

r=dbradley

Yes it's a nice solution. Eliminates some knowledge about the interfaces
involved and skirts the issue altogether.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 176415 [details] [diff] [review]
Simpler patch

sr=dmose

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Patch checked in.  Thanks, sharparrow1.