Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GCC build warning: sps_sampler.h: In member function ‘void ProfileStack::push(const char*)’: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] (also in addMarker)

RESOLVED FIXED in mozilla14

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: Jacek Caban)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Filing bug on this new build warning:

sps_sampler.h: In member function ‘void ProfileStack::addMarker(const char*)’:
sps_sampler.h:187:56: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
sps_sampler.h: In member function ‘void ProfileStack::push(const char*)’:
sps_sampler.h:216:53: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

This is from these lines:
> if (mMarkerPointer == mozilla::ArrayLength(mMarkers)) {
and
> if (mStackPointer >= mozilla::ArrayLength(mStack)) {

ArrayLength returns size_t (unsigned), while mStackPointer and mMarkerPointer are of type "mozilla::sig_safe_t" which is apparently signed.
(Reporter)

Comment 1

5 years ago
Actually it looks like sig_atomic_t could be signed or unsigned depending on the platform.

Perhaps we should cast the ArrayLength return-value to sig_atomic_t before doing the comparison? (assuming that we don't expect to have an array large enough that its size would overflow when converted to an unsigned value)

[caveat: I haven't looked at this in much detail, so my thoughts may be way off at this point]
(Reporter)

Comment 2

5 years ago
Since this sps_sampler.h is included (directly or indirectly) in a number of places, this bug is responsible for 76 lines of build warnings during a Linux clobber build, making it reasonably-spammy, as build warning bugs go.

(based on  "grep 'sps_sampler.h:' | wc -l" in this log from a free-space clobber:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=10221703&tree=Firefox )
(Assignee)

Comment 3

5 years ago
Created attachment 607889 [details] [diff] [review]
fix v1.0

Values of these sig_safe_t variables will never overflow AFAICS since they will never be larger than ArrayLength, which is constant. I think casting these values is slightly more appropriate than casting ArrayLength here because their true meaning is what size_t is for (although it's just a matter of taste).
Assignee: nobody → jacek
Attachment #607889 - Flags: review?(ehsan)
Attachment #607889 - Flags: review?(ehsan) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f521e78ca782
Whiteboard: [inbound]

Updated

5 years ago
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/f521e78ca782
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.