As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 754488 - Build warnings in FrameLayerBuilder.cpp: "warning: comparison between signed and unsigned integer expressions [-Wsign-compare]", " warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]"
: Build warnings in FrameLayerBuilder.cpp: "warning: comparison between signed ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: buildwarning GPU-clipping-rounded
  Show dependency treegraph
 
Reported: 2012-05-11 16:04 PDT by Daniel Holbert [:dholbert]
Modified: 2012-05-15 06:33 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove the second warning (989 bytes, patch)
2012-05-13 17:31 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch (2.31 KB, patch)
2012-05-13 23:33 PDT, Nick Cameron [:nrc]
dholbert: review+
Details | Diff | Splinter Review
patch (3.26 KB, patch)
2012-05-14 14:12 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] 2012-05-11 16:04:31 PDT
Bug 716439 added 2 new build warnings in FrameLayerBuilder.cpp:
{
../../../mozilla/layout/base/FrameLayerBuilder.cpp: In member function 'void mozilla::{anonymous}::ContainerState::ThebesLayerData::UpdateCommonClipCount(const mozilla::FrameLayerBuilder::Clip&)':
../../../mozilla/layout/base/FrameLayerBuilder.cpp:1154:56: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
../../../mozilla/layout/base/FrameLayerBuilder.cpp: In member function 'void mozilla::FrameLayerBuilder::Clip::ApplyRoundedRectsTo(gfxContext*, PRInt32, PRUint32, PRUint32) const':
../../../mozilla/layout/base/FrameLayerBuilder.cpp:2651:24: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
}

Both of these are in assertions, indicating that we might not be checking what we think we're checking in those cases.

(I'm building w/ GCC 4.6.3)
Comment 1 User image Nick Cameron [:nrc] 2012-05-13 03:45:33 PDT
I checked the code, the first warning is not a problem, I think (but will double check), mCommonClipCount is negative when not initialised, but here is compared to the length of a list. I will see if the warning can be avoided somehow.

The second assertion is superfluous, I think it is a left over from an earlier version of the code. I'll make a patch to remove it asap.
Comment 2 User image Nick Cameron [:nrc] 2012-05-13 17:30:23 PDT
To confirm, the first warning is unavoidable because mCommonClipCount is -1 initially, but is safe because we are inside an 'if (mCommonClipCount >= 0)' block.
Comment 3 User image Nick Cameron [:nrc] 2012-05-13 17:31:46 PDT
Created attachment 623541 [details] [diff] [review]
remove the second warning

I assume this will be OK without a Try run?
Comment 4 User image Daniel Holbert [:dholbert] 2012-05-13 19:37:03 PDT
Comment on attachment 623541 [details] [diff] [review]
remove the second warning

Following the possible-callers of this method up, I found:
> 2399 FrameLayerBuilder::DrawThebesLayer(ThebesLayer* aLayer,
[...]
> 2404 {
[...]
> 2414   PRInt32 commonClipCount;
[...]
> 2537         currentClip.ApplyTo(aContext, presContext, commonClipCount);

(where ApplyTo is a wrapper for ApplyRoundedRectsTo, sort of)

So commonClipCount is a signed value that we're converting to unsigned, without ever checking that it's nonnegative, AFAICT.  So we could still potentially end up with a (wrapped) negative value at the place your assertion touches.

Assuming I'm reading this correctly -- rather than dropping the assertion entirely, could you bump it up a few stack-levels to just before that ApplyTo call?

Also, this patch only addresses one of the two warnings -- could you address the other one noted in comment 0 ("comparison between signed and unsigned integer expressions") as well?
Comment 5 User image Daniel Holbert [:dholbert] 2012-05-13 19:39:24 PDT
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Also, this patch only addresses one of the two warnings -- could you address
> the other one noted in comment 0 ("comparison between signed and unsigned
> integer expressions") as well?

oh, sorry, I missed comment 2.

Could swap "mCommonClipCount" for "PRUint32(mCommonClipCount)" in that assertion, then? (perhaps with a comment noting that we're in a mCommonClipCount>=0 clause, if you like)
Comment 6 User image Daniel Holbert [:dholbert] 2012-05-13 19:39:41 PDT
*Could we swap
Comment 7 User image Nick Cameron [:nrc] 2012-05-13 23:32:18 PDT
The conversion from signed to unsigned is at line 1285, and is checked at line 1280. This is because there are two mCommonClipCounts in two different classes, one signed, one unsigned. The latter is used in DrawThebesLayer, there was a mistake in the signedness of commonClipCount that is corrected in the new patch.
Comment 8 User image Nick Cameron [:nrc] 2012-05-13 23:33:38 PDT
Created attachment 623596 [details] [diff] [review]
patch
Comment 9 User image Daniel Holbert [:dholbert] 2012-05-14 09:44:28 PDT
Comment on attachment 623596 [details] [diff] [review]
patch

Ah, makes sense.

Side note: I noticed that the _unsigned_ version of mCommonClipCounts is initialized to -1, which seems odd:
> 533   class ThebesLayerItemsEntry : public nsPtrHashKey<ThebesLayer> {
> 534   public:
> 535     ThebesLayerItemsEntry(const ThebesLayer *key) :
> 536         nsPtrHashKey<ThebesLayer>(key), mContainerLayerFrame(nsnull),
> 537         mHasExplicitLastPaintOffset(false), mCommonClipCount(-1) {}
[...]
> 554     PRUint32 mCommonClipCount;

Perhaps that should be initialized to PR_UINT32_MAX instead of -1, to be clearer about its valid range?  (of course, they compute to the same value)

Either way, r=me
Comment 10 User image Nick Cameron [:nrc] 2012-05-14 14:12:20 PDT
Created attachment 623806 [details] [diff] [review]
patch

Initialising ThebesLayerEntry::mCommonClipCount to 0, otherwise unchanged. Carrying r=dholbert
Comment 11 User image Daniel Holbert [:dholbert] 2012-05-14 14:29:49 PDT
(Cool -- I'll take your word for it that 0 is a saner initial value than -1 was. :) )
Comment 12 User image Ryan VanderMeulen [:RyanVM] 2012-05-14 16:02:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/78ab86f18887
Comment 13 User image Ed Morley [:emorley] 2012-05-15 06:33:46 PDT
https://hg.mozilla.org/mozilla-central/rev/78ab86f18887

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