117522 - gfx/src/nsCompressedCharMap.cpp doesn't build: too many Ls in nsCompressedCharMap.h

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[dan]

dist/include/gfx/nsCompressedCharMap.h contains the line 

#   define CCMAP_POW2(n)           (1LL<<(n))

gcc 2.95.4 on Linux 2.4.17-rc1 refuses to compile this, complaining about too
many `L' characters.  The workaround appears to be to remove one of the Ls -
though note that it hasn't actually finished building yet, so perhaps that will
cause it die horribly at some later point.  Someone who knows what they're doing
might have a better idea

For reference, "long" and "long long" are the same size (64 bits) on Alpha/Linux

:; cvs up -dPr  MOZILLA_0_9_7_BRANCH
:; ./configure  --disable-debug --disable-mailnews --enable-crypto
--enable-optimize --disable-ldap
:; make

Comment 1

[Boris Zbarsky [:bzbarsky]]

Over to intl

Comment 2

[Chase Tingley]

*** Bug 117545 has been marked as a duplicate of this bug. ***

Comment 3

[kill this account]

long and long long are different on Intel PC Linux.

This code has been in since Sept 18, 2001. 
Do we know why this is problem 10 weeks later?

Comment 4

[Frank Tang]

add xp guru jdunn to the cc list.

Comment 5

[Jim Dunn]

I would suggest grabbing a 0.9.6 & a 0.9.7 tree.
build on each.  For a .cpp that includes this nsCompressedCharMap.h
preprocess that file and figure out what is going on.

1LL<<(n) seems to me to be a SCARY macro.
You mention a long long on Alpha/Linux is 64.
What about on Tru64?  Or is that what you mean by Alpha.

Comment 6

[Shanjian Li]

Attachment: a possible fix

Comment 7

[Shanjian Li]

I just proposed a patch. But I would like to get answer of the following questions 
before we go any further. 
1) (from bstell) why this is a problem 10 wks after the code was checked in?
2) On what platform (hardware/software), with which version of compiler do we have such problem?
3) Is there another version of compiler possible to support "LL" on that platform?

It is rather strange to me for a compiler running on 64bits architecture not supporting 
"LL".

Comment 8

[Brendan Eich [:brendan]]

1LL << n is not portable.  If you want to make a 64-bit integer with 1 bit set,
use prlong.h and use LL_SHL.  This is ugly, but should (cross fingers) compile
as well on platforms that support 64-bit integers.

/be

Comment 9

[timp]

I'll try to answer the questions in comment 7
 
1.) This bug first appeared with the 0.9.6 release.  0.9.5 had
#define ALU_SIZE 16 patched in nsCompressedCharMap.h just before the 1LL<<(n)
section of the code.
 
Sorry I didn't speak up when it first broke on my system.
 
2.) It breaks on my Alpha 164SX running Debian Linux using:
gcc version 2.95.4 20011006 (Debian prerelease).
 
3.) The only other compiler for Alpha Linux is the Compaq's True 64 Unix
compiler, however most Linux distros ship with gcc due to license issues.

Comment 10

[Shanjian Li]

Using LL_SHL will be a little complicate to implement, since LL_SHL 
has the form of LL_SHL(r, a, b). I believe my approach should be good 
enough, since we already know ALU_TYPE is 64bits. It should be safe to 
assume 64bits SHL operation is available. 

We are planning to extend the use CCMAP, and it is rather difficult to 
initiate a static ccmap with multiple ALU_TYPE interpretations. So I am 
thinking of change that. If that is the case, this bug will be gone. 
In the mean time, please use my patch to temporarily fix your local tree.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 63511 [details] [diff] [review]
a possible fix

shanjian, good point -- but no need for the L after the 1, given the cast to
(PRUint64).  Remove that L and sr=[e-mail].

/be

Comment 12

[timp]

I tried building 0.9.7 with the patch (attachment 63511 [details] [diff] [review]) and the compiler
stops with:

nsCompressedCharMap.cpp:318: parse error
nsCompressedCharMap.cpp:322: parse error
nsCompressedCharMap.cpp:339: parse error

I tried the patch as is, then removed the 'L' after the 1 and the results
were the same.

System: Linux Alpha 164SX, Debian Woody, gcc version 2.95.4 20011006 (Debian
prerelease)

Comment 13

[Brendan Eich [:brendan]]

Oops, #if and #elif tests -- the C preprocessor can't handle casts.  What to do?
 Perhaps don't use CCMAP_POW2, just use PR_BIT, when defining
CCMAP_BITS_PER_ALU. That macro never needs more than an int's worth of bits.

/be

Comment 14

[Shanjian Li]

Since "ALU_SIZE" is typedef as "PR_BITS_PER_LONG", it should be safe to 
just say:
#   define ALU_TYPE                PRUint64
#   define CCMAP_POW2(n)           (1L<<(n))
#   define CCMAP_BITS_PER_ALU_LOG2 6

Reporter, could you try this?

Comment 15

[timp]

Milestone 0.9.7 and 0.9.8 both compile and run ok with-

#elif (ALU_SIZE==64)
#   define ALU_TYPE                PRUint64
//#   define CCMAP_POW2(n)           (1LL<<(n))
#   define CCMAP_POW2(n)           (1L<<(n))
#   define CCMAP_BITS_PER_ALU_LOG2 6

I've been running 0.9.7 for a couple of weeks like this with no problems.

Tim

Comment 16

[kill this account]

Brendan: I'm not a 64 bit expert in any sense. Could you indicate who
would be a good candidate to decide how to properly handle this?

Comment 17

[hacker formerly known as [e-mail]]

*** Bug 124188 has been marked as a duplicate of this bug. ***

Comment 18

[Shanjian Li]

Attachment: the patch that has been tested.

Comment 19

[Shanjian Li]

For reasons as I mentioned in comment #14, the new patch should fix the problem
without creating new one. 

bstell, brendan, could you r/sr?

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 69358 [details] [diff] [review]
the patch that has been tested.

sr=[e-mail]

Comment 21

[kill this account]

To me the patch looks okay but I am not an expert on 64 bit issues.

Since the patch is okay with Brendan I am okay rubber stamping an r= so
it meets the checkin rules.

Comment 22

[Shanjian Li]

fix checked in. 

Comment 23

[Teruko Kobayashi]

Verified as fixed.