1107946 - Build warning (treated as error) pkixnames_tests.cpp:1077:22: error: unused variable 'ipv4_other_addr_str' [-Werror,-Wunused-const-variable], and similar

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Daniel Holbert [:dholbert]]

New build warnings, treated as errors (breaking my local build), in clang 3.5 on linux:
{
$SRCDIR/security/pkix/test/gtest/pkixnames_tests.cpp:1077:22: error: unused variable 'ipv4_other_addr_str' [-Werror,-Wunused-const-variable]
static const uint8_t ipv4_other_addr_str[] = "5.6.7.8";
                     ^

$SRCDIR/security/pkix/test/gtest/pkixnames_tests.cpp:1153:22: error: unused variable 'ipv4_constraint_truncated_bytes' [-Werror,-Wunused-const-variable]
static const uint8_t ipv4_constraint_truncated_bytes[] = {
                     ^

$SRCDIR/security/pkix/test/gtest/pkixnames_tests.cpp:1157:22: error: unused variable 'ipv4_constraint_overlong_bytes' [-Werror,-Wunused-const-variable]
static const uint8_t ipv4_constraint_overlong_bytes[] = {
                     ^

$SRCDIR/security/pkix/test/gtest/pkixnames_tests.cpp:1174:22: error: unused variable 'ipv6_constraint_truncated_bytes' [-Werror,-Wunused-const-variable]
static const uint8_t ipv6_constraint_truncated_bytes[] = {
                     ^

$SRCDIR/security/pkix/test/gtest/pkixnames_tests.cpp:1180:22: error: unused variable 'ipv6_constraint_overlong_bytes' [-Werror,-Wunused-const-variable]
static const uint8_t ipv6_constraint_overlong_bytes[] = {
                     ^
}
5 errors generated.

Looks like these were introduced in bug 970542 -- particularly in parts 7 and 8.

Brian, did you intend to use these variables? If so, could you add the usages that you intended to include? (Or if not, could you remove the variables?)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I will fix this now. Thanks for letting me know about this.

FWIW, I do build with warnings-as-errors locally, but on Windows instead of Linux. If there's a way to make MSVC work more like the Linux compilers with respect to this type of issue, please let me know so that this problem can be avoided in the future.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: remove-unused-variables.patch

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> I will fix this now. Thanks for letting me know about this.

Thanks for the quick turnaround! I'm building with this locally now to sanity-check it. Looks good from skimming the patch visually.

> FWIW, I do build with warnings-as-errors locally, but on Windows instead of
> Linux. If there's a way to make MSVC work more like the Linux compilers with
> respect to this type of issue, please let me know

I don't think there is a way to do that, unfortunately. The general problem here is new compiler-versions adding new (useful!) warnings, which aren't discoverable until someone tries to build with that new compiler-version.

There's no general solution to that problem; the only thing we can do is keep TBPL builders from getting too out of date (so we'll discover stuff there & turn the build red -- that didn't happen in this case, b/c TBPL is on semi-old clang). Without that, we just have to rely on people like me filing bugs.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8532624 [details] [diff] [review]
remove-unused-variables.patch

All right, I've confirmed that this fixes my build. Thanks!

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8532624 [details] [diff] [review]
remove-unused-variables.patch

Review of attachment 8532624 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed (i.e. I think some of these values were intended to be used in some tests where other values were unintentionally used. If that's the case, we should just use them. If not, then they can be removed.)

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ -1149,5 @@
>  };
>  static const uint8_t ipv4_addr_overlong_bytes[] = {
>    1, 2, 3, 4, 5
>  };
> -static const uint8_t ipv4_constraint_truncated_bytes[] = {

There's a test with a comment that says "The presented IPv4 constraint is truncated" (similarly with ipv4_constraint_overlong_bytes). Was this intended to be used there?

@@ -1170,5 @@
>    0x55, 0x66, 0x77, 0x88,
>    0x99, 0xaa, 0xbb, 0xcc,
>    0xdd, 0xee, 0xff, 0x11, 0x00
>  };
> -static const uint8_t ipv6_constraint_truncated_bytes[] = {

Same question as with the ipv4 constraints.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-build-warnings.patch

This fixes the bug in the way that keeler suggested.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0300c3cdb668

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57760cba1402

Comment 12

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/57760cba1402