123403 - LL_INIT should use PR_UINT32, not PR_INT32

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[timeless]

The destination fields are Unsigned, so we shouldn't cast them to signed in the interim.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtypes.h&rev=3.21&mark=342,344#339

Comment 1

[Wan-Teh Chang]

This is a simple change, but what is the problem?
Does some compiler warn about signed/unsigned mismatch?

Comment 2

[Jan Ruzicka]

No compiler does not warn you about anything, but it silently does a sign
extension. 

try :

PRInt64 minus_one_a = LL_INIT(0,-1);
PRInt64 minus_one_b = LL_INIT(-1,-1);
PRInt64 minus_one_hi_and_lo = LL_INIT(0xFFFFFFFF,0xFFFFFFFF);
PRInt64 minus_one_only_lo = LL_INIT(0,0xFFFFFFFF);

/* test one if initialication is portable as it claims 
this test should pass on both systems with and without native 64 bit integers */ 
if ( LL_EQ(minus_one_a, minus_one_hi_and_lo) )
  printf "test 1 failed - (0,-1) initializes both hi and lo portions!\n" 
else
  printf "test 1 passed\n" 

/* test two if initialication is portable as it claims
this test should pass on both systems with and without native 64 bit integers */ 
if ( LL_EQ(minus_one_b, minus_one_hi_and_lo) )
  printf "test 2 passed\n" 
else
  printf "test 2 failed - (-1,-1) initialization is not working as expected!\n" 

Comment 3

[timeless]

Attachment: per comment 0

Comment 4

[Wan-Teh Chang]

The sign extension problem that Jan Ruzicka pointed
out in comment 2 is a different problem because it
only affects platforms where HAVE_LONG_LONG is defined.
The cause of the problem is that you cannot pass negative
integers as arguments to the LL_INIT macro.

timeless, is there any currently supported platform
where HAVE_LONG_LONG is undefined?  I am wondering
how we can test your patch.  I have already tested
it with NSPR but would like to test it with Mozilla,
too.

Comment 5

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

(In reply to comment #4)
> The cause of the problem is that you cannot pass negative
> integers as arguments to the LL_INIT macro.

How am I supposed to init a PRInt64 with -1, then?
I'd rather not do it by substracting 1 from zero...

I currently use LL_INIT(0, -1). that will obviously not work when there is no
native 64bit int type.

Comment 6

[Wan-Teh Chang]

If it is a global or static variable, you should say:

PRInt64 minus_one = LL_INIT(0xffffffff, 0xffffffff);

If it is a local variable, you should say:

    PRInt64 minus_one;

    minus_one = LL_I2L(-1);

Comment 7

[Wan-Teh Chang]

Patch checked into the NSPR trunk (NSPR 4.6) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).

Since all currently supported platforms define
HAVE_LONG_LONG, this patch is effectively a no-op.
I did try undefining HAVE_LONG_LONG on Linux and
Solaris and building NSPR.  I didn't see any
compiler warnings before the patch was applied.
All NSPR tests (in particular lltest.c) passed
after the patch was applied.

Comment 8

[Wan-Teh Chang]

"no-op" is not the right word.  It should say
"this patch is effectively modifying dead code".