194187 - 64bit unclean code in jsemit.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Philipp Thomas]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de-AT; rv:1.2.1) Gecko/20021204
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; de-AT; rv:1.2.1) Gecko/20021204

jsemit.h has this:

#define BITS_PER_PTRDIFF        (sizeof(ptrdiff_t) * JS_BITS_PER_BYTE)
#define BITS_PER_BPDELTA        (BITS_PER_PTRDIFF - 1 - JT_UNTAG_SHIFT)
#define BPDELTA_MAX             ((ptrdiff_t)(JS_BIT(BITS_PER_BPDELTA) - 1))

And jstypes.h has:

/***********************************************************************
** MACROS:      JS_BIT
**              JS_BITMASK
** DESCRIPTION:
** Bit masking macros.  XXX n must be <= 31 to be portable
***********************************************************************/
#define JS_BIT(n)       ((JSUint32)1 << (n))
#define JS_BITMASK(n)   (JS_BIT(n) - 1)

Which of course leads to gcc warning:
   warning: left shift count >= width of type

when actually using BPDELTA_MAX, as shifting a JSUint32 by 64 bits definitely
exceeds its size.

I'd propose to change the definition of BPDELTA_MAX to

#define BPDELTA_MAX             ((ptrdiff_t)(((prtdiff_t)1 << BITS_PER_BPDELTA)
- 1))


Reproducible: Always

Steps to Reproduce:
Just look at the code :)

Comment 1

[Philipp Thomas]

I forgot to state that this comes from compiling Mozilla on x86-64, aka AMD 
Hammer, which uses the LP64 model, i.e. long and pointer are 64 bit long, so
sizeof(ptrdiff_t) is 8.

Comment 2

[Andrew Schultz]

same fun on Alpha/Linux

Comment 3

[Phil Schwartau]

Reassigning; cc'ing Brendan -

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Oops.  Sorry about that.

I don't think two casts are needed -- the inner (ptrdiff_t) cast applied to 1
should suffice to yield the right type for the entire expression that is the
macro's definition.  Please let me know if this patch is good soon, and I will
try to get it into 1.3 final.  Thanks,

/be

Comment 5

[Brendan Eich [:brendan]]

I'll take this.

/be

Comment 6

[Brendan Eich [:brendan]]

This is an easy portability fix to get into 1.3final.

/be

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 115025 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to 1.3

Comment 8

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 9

[Phil Schwartau]

Marking Verified -