Closed Bug 1083373 Opened 10 years ago Closed 10 years ago

Build warnings in media/gmp-clearkey

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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;
                    ^
}
(I'm building with with clang 3.5 on Ubuntu 14.10, btw)
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: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8512048 - Flags: review?(edwin)
(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)
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)
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)
(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.)
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.
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
Keywords: leave-open
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
That Try run looked good, so I landed the amended part 2:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4d7a689070
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: