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]

RESOLVED FIXED in 4.13

Status

defect
--
trivial
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: julien.pierre, Assigned: wtc)

Tracking

(Blocks 1 bug)

4.13
Sun
Solaris
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected)

Details

(Whiteboard: [build_warning])

Attachments

(2 attachments)

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 ...
OS: SunOS → Solaris
QA Contact: wtchang → nspr
Blocks: buildwarning
Whiteboard: [build_warning]
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
Flags: needinfo?(julien.pierre)
Summary: compiler warnings related to LL_INIT → 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]
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().
Attachment #8701223 - Flags: superreview?(ted)
Attachment #8701223 - Flags: review?(kaie)
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.
Attachment #8701223 - Flags: superreview?(ted)
Attachment #8701223 - Flags: superreview+
Attachment #8701223 - Flags: review?(kaie)
Flags: needinfo?(julien.pierre)
(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?
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.)
Keywords: checkin-needed
Target Milestone: --- → 4.13
Is this ready to land now?
Flags: needinfo?(wtc)
Do we want this or should we WONTFIX?
Flags: needinfo?(kaie)
Finally I saw this again, and checked it in.
https://hg.mozilla.org/projects/nspr/rev/d3820777dd94
Flags: needinfo?(wtc)
Flags: needinfo?(kaie)
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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.
Attachment #8701223 - Flags: checked-in+
You need to log in before you can comment on or make changes to this bug.