603471 - int32* and int32_t* are not inter-convertible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

On Windows, int32 is typedef'd to long while int32_t is typedef'd to the compiler's 32-bit integer intrinsic. This means that passing an int32* to a function taking int32_t* causes a compiler error. The codebase contains a mixture of int32 and int32_t, so this cases problems. The problem is especially annoying because it's only a problem on Windows.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

It's not entirely clear how to fix this problem. Fixing them all in one pass would rot a lot of people's patches unnecessarily. This patch makes int32 and int32_t be the same type, so that we can fix the problem incrementally.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I forgot to say: I have no idea why there was a special case for Windows in the first place. The patch seems to work now, but it would be nice if someone knew.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> I forgot to say: I have no idea why there was a special case for Windows in the
> first place. The patch seems to work now, but it would be nice if someone knew.

My understanding is that it's due to Windows lacking stdint.h.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #3)
> My understanding is that it's due to Windows lacking stdint.h.

OK, I didn't know that. That's lame.

It looks like jsinttypes.h, which provides a sort of platform-independent version of stdint.h, is newer than the definitions of int32 and uint32 in jsotypes.h. So that might explain things.

Comment 5

[Jeff Walden [:Waldo]]

I think comment 3 was referring to the special case to provide definitions for int32_t and friends -- *not* for the special case in making int32 long versus JSInt32.  So I think the patch here is still good to go.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: updated patch

The previous patch worked for the JS engine, but the browser failed to build. This fixes that problem.

The issue was that some browser code was calling js_CreateArrayBuffer, which takes a jsuint. We typedef'd jsuint to a uint32. Unfortunately, the browser has its own definition of uint32 (at least in Windows). The result was a link error.

I've changed jsint and jsuint to be JSInt32/JSUint32, since these are supposed to be platform-independent and they can be exposed to code outside of the JS engine.

I'm still not sure this is 100% the right thing to do, but it seems to work.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 482581 [details] [diff] [review]
updated patch

Jim did the stdint/inttypes veneering and is a better reviewer for this patch.

/be

Comment 8

[David Mandelin [:dmandelin]]

Can we get an expedited review on this? The patch is tiny and it blocks key perf work on trace tuning.

Comment 9

[Jim Blandy :jimb]

Reviewing...

Comment 10

[Jim Blandy :jimb]

Looks good to me.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/7a451e204442

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7a451e204442