737667 - 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)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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]

Comment 2

[Daniel Holbert [:dholbert]]

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 )

Comment 3

[Jacek Caban]

Attachment: 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).

Comment 4

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f521e78ca782

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f521e78ca782