Closed
Bug 1083373
Opened 10 years ago
Closed 10 years ago
Build warnings in media/gmp-clearkey
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
5.21 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
996 bytes,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
I get four build warnings when building in media/gmp-clearkey: { $SRCDIR/media/gmp-clearkey/0.1/ClearKeySession.cpp:18:5: warning: field 'mHost' will be initialized after field 'mCallback' [-Wreorder] , mHost(aHost) ^ $SRCDIR/media/gmp-clearkey/0.1/ClearKeyUtils.cpp:106:23: warning: array subscript is of type 'char' [-Wchar-subscripts] out[i] = sAlphabet[out[i]]; ^~~~~~~ $SRCDIR/media/gmp-clearkey/0.1/ClearKeyUtils.cpp:95:21: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare] for (int i = 0; i < aEncoded.length(); i++) { ~ ^ ~~~~~~~~~~~~~~~~~ In file included from $SRCDIR/media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:10: In file included from $SRCDIR/media/gmp-clearkey/0.1/ClearKeyDecryptionManager.h:12: $SRCDIR/media/gmp-clearkey/0.1/ClearKeySession.h:37:21: warning: private field 'mHost' is not used [-Wunused-private-field] GMPDecryptorHost* mHost; ^ }
Assignee | ||
Comment 1•10 years ago
|
||
(I'm building with with clang 3.5 on Ubuntu 14.10, btw)
Assignee | ||
Comment 2•10 years ago
|
||
This patch fixes the 1st & 4th build warnings, by removing the unused variable ClearKeySession::mHost. The code that instantiates ClearKeySession -- ClearKeyDecryptionManager -- has its own version of "mHost", which becomes unused as well after that change. So, this path removes that now-unused variable, too.
Assignee | ||
Comment 3•10 years ago
|
||
(Sorry, forgot to qref in a local tweak in one of the modified lines, to adjust the position of a "*" in a function-param, for consistency. Fixed now.)
Attachment #8512048 -
Attachment is obsolete: true
Attachment #8512048 -
Flags: review?(edwin)
Attachment #8512050 -
Flags: review?(edwin)
Assignee | ||
Comment 4•10 years ago
|
||
This patch fixes the second warning, by casting "out[i]" to a size_t (instead of a char) when using it as an array index. I have no idea if this cast is actually valid, but if it's not, we're in trouble because that means we're indexing off the start of the array. (That's what this build warning is concerned with, I think.) So, to defend against that, I've also added an assertion to verify (in debug builds at least) that our indexing is in-bounds.
Attachment #8512056 -
Flags: review?(edwin)
Assignee | ||
Comment 5•10 years ago
|
||
And this patch fixes the third warning by making us use string::size_type as our loop variable. (Per comment 0, "size_type" -- for 'string' -- is the type that we're comparing against, in the < aEncoded.length() comparison; so, it's the type we should use for our loop variable as well, to be sure the comparison is valid.)
Attachment #8512059 -
Flags: review?(edwin)
Assignee | ||
Comment 6•10 years ago
|
||
(Side note: unfortunately, we can't mark this directory FAIL_ON_WARNINGS right now, because it includes the openaes library in its moz.build file, and that library has its own pile of build warnings. If we split off openaes to have its own moz.build file, though, then we could mark this mozilla code as warning-free while leaving the openaes code in its own warningful directory.)
Attachment #8512050 -
Flags: review?(edwin) → review+
Attachment #8512056 -
Flags: review?(edwin) → review+
Attachment #8512059 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e6e78b0338 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e523e3d339 https://hg.mozilla.org/integration/mozilla-inbound/rev/2abc300df08c
Flags: in-testsuite-
Assignee | ||
Comment 8•10 years ago
|
||
Apparently this caused an Android-only warning-as-errors bustage (for a warning that we globally treat as an error, regardless of per-directory FAIL_ON_WARNINGS settings): { /builds/slave/m-in-and-d-0000000000000000000/build/media/gmp-clearkey/0.1/ClearKeyUtils.cpp: In function 'bool EncodeBase64Web(std::vector<unsigned char>, std::string&)': /builds/slave/m-in-and-d-0000000000000000000/build/media/gmp-clearkey/0.1/ClearKeyUtils.cpp:112:80: error: comparison is always true due to limited range of data type [-Werror=type-limits] /builds/slave/m-in-and-d-0000000000000000000/build/media/gmp-clearkey/0.1/ClearKeyUtils.cpp:112:244: error: comparison is always true due to limited range of data type [-Werror=type-limits] } https://tbpl.mozilla.org/php/getParsedLog.php?id=51172512&tree=Mozilla-Inbound Line 112 is the line that that patch changed to this: > for (string::size_type i = 0; i < aEncoded.length(); i++) { The reported warning makes no sense for this line, though, so I suspect it's really referring to the conditions in the MOZ_ASSERT in part 2. (Maybe 'char' is unsigned on Android, so the >=0 comparison is always true, or something.) I'm backing out parts 2 and 3 and I'll diagnose further on Try.
Assignee | ||
Comment 9•10 years ago
|
||
Oh, I misread the patch as putting line 112 at a different point. Looking at the annotated file[1], it's clear that line 112 is indeed the MOZ_ASSERT. So, I'm just backing out part 2. [1] https://hg.mozilla.org/integration/mozilla-inbound/annotate/c7e523e3d339/media/gmp-clearkey/0.1/ClearKeyUtils.cpp#l112
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b93a000dfb
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•10 years ago
|
||
cdleary's blog says that 'char' is unsigned on ARM, which explains why the bustage is Android/B2G-only issue (with the compiler telling us that checking a (implicitly-unsigned) char for negativeness is silly & useless on those platforms): http://blog.cdleary.com/2012/11/arm-chars-are-unsigned-by-default/ confirms that I intend to just drop the negativeness check, & just check whether the static_cast<size_t> value is larger than the array size. Any negative (implicitly-signed) char value would end up failing that check, since it'd be converted to an unsigned value >= 128, which is still out-of-bounds for this array. Try run to sanity-check this option: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fd58ebde0d95
Assignee | ||
Comment 12•10 years ago
|
||
That Try run looked good, so I landed the amended part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4d7a689070
Keywords: leave-open
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a6e6e78b0338 https://hg.mozilla.org/mozilla-central/rev/2abc300df08c https://hg.mozilla.org/mozilla-central/rev/8d4d7a689070
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•