Last Comment Bug 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)
: GCC build warning: sps_sampler.h: In member function ‘void ProfileStack::push...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Jacek Caban
:
:
Mentors:
Depends on:
Blocks: buildwarning 735191
  Show dependency treegraph
 
Reported: 2012-03-20 15:30 PDT by Daniel Holbert [:dholbert]
Modified: 2012-03-22 06:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (1.47 KB, patch)
2012-03-21 03:11 PDT, Jacek Caban
ehsan: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-03-20 15:30:05 PDT
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] 2012-03-20 15:35:08 PDT
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] 2012-03-20 16:30:46 PDT
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 2012-03-21 03:11:53 PDT
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).
Comment 5 Marco Bonardo [::mak] 2012-03-22 06:26:15 PDT
https://hg.mozilla.org/mozilla-central/rev/f521e78ca782

Note You need to log in before you can comment on or make changes to this bug.