265368 - LL_INIT warning: signed shift result (0x8000000000000000) sets the sign bit of the shift expression's type ('long long') and becomes negative [-Wshift-sign-overflow]

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Julien Pierre]

The following warnings occur on all Solaris platforms (sparc32/sparc64/x86/AMD64) :

"../../../../pr/src/misc/prlong.c", line 39: warning: integer overflow detected:
op "<<"
"../../../../pr/src/misc/prlong.c", line 40: warning: integer overflow detected:
op "<<"
"../../../../pr/src/misc/prlong.c", line 40: warning: initializer does not fit
or is out of range: -1

Lines 39 and 40 are respectively .
static PRInt64 ll_minint = LL_INIT( 0x80000000, 0x00000000 );
static PRUint64 ll_maxuint = LL_INIT( 0xffffffff, 0xffffffff );

The warnings occur only when trying to set the highest bit . I don't think they
denote an actual problem with the code, but it would be nice to be able to come
up with a definition for LL_INIT that works and doesn't generate warnings.

I believe it may be possible to eliminate the first warning through clever use
of casting inside the LL_INIT macro definition.

As for eliminating the second warning, it may require a separate ULL_INIT macro ...

Comment 1

[Chris Peterson [:cpeterson]]

Julien, do you still see these LL_INIT warnings with the Sun compiler?

I can reproduce these warnings with clang on OS X if I enable clang's off-by-default -Wshift-sign-overflow warning:

warning: signed shift result (0x8000000000000000) sets the sign bit of the shift expression's type ('long long') and becomes negative [-Wshift-sign-overflow]

warning: signed shift result (0xFFFFFFFF00000000) sets the sign bit of the shift expression's type ('long long') and becomes negative [-Wshift-sign-overflow]

Fixing this warning is a very low priority because clang -Wshift-sign-overflow is disabled by default and LL_INIT is only used in a few places in NSPR and NSS:

https://mxr.mozilla.org/mozilla-central/ident?i=LL_INIT

Comment 2

[Wan-Teh Chang]

Attachment: Fix the LL_INIT() macro.

Chris: thank you very much for looking at this bug and suggesting an easy way
to reproduce the compiler warning. This bug was rediscovered recently in
bug 1180095. I attached a patch, which shows the fix may break some
workarounds for this bug and therefore require undoing the workarounds.
So I'm not sure if we should fix this bug.

The best workaround available today to replace LL_INIT() with PR_INT64() or
PR_UINT64().

Comment 3

[Wan-Teh Chang]

Attachment: Use PR_INT64() or PR_UINT64() instead of LL_INIT()

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8701223 [details] [diff] [review]
Use PR_INT64() or PR_UINT64() instead of LL_INIT()

Review of attachment 8701223 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine. I can't imagine we actually build anywhere that doesn't support 64-bit integer literals.

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #2)
> 
> I attached a patch, which shows the fix may break some
> workarounds for this bug and therefore require undoing the workarounds.

Which specific workarounds might this patch break?

Since this patch changes how these 4 constants are being defined, but doesn't change any macros, I don't understand how this could affect any other code.

Which workarounds do you think would have to be undone?

And if there is anything that needs to be undone - shouldn't this patch be combined with a patch that includes the undos that you are referring to, to ensure everything gets landed at the same time?

Comment 6

[Kai Engert (:KaiE:)]

To, me the patch looks fine, too.

Unless we get an explanation, I'd suggest to land this soon.

(However, we should postpone 1-2 weeks, because NSPR is frozen for release preparations currently.)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this ready to land now?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Do we want this or should we WONTFIX?

Comment 9

[Kai Engert (:KaiE:)]

Finally I saw this again, and checked it in.
https://hg.mozilla.org/projects/nspr/rev/d3820777dd94

Comment 10

[Wan-Teh Chang]

Comment on attachment 8701223 [details] [diff] [review]
Use PR_INT64() or PR_UINT64() instead of LL_INIT()

Review of attachment 8701223 [details] [diff] [review]:
-----------------------------------------------------------------

Ryan: I'm very sorry I didn't respond.

Kai: thank you very much for checking in this patch.