Closed Bug 737667 Opened 8 years ago Closed 8 years ago

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)

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: dholbert, Assigned: jacek)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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]
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 )
Attached patch fix v1.0Splinter Review
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+
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/f521e78ca782
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.