Opened 3 years ago

Closed 3 years ago

#19180 closed defect (implemented)

Add new compiler warnings

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-proposed
Cc: Actual Points: 4
Parent ID: Points: 4
Reviewer: Sponsor: SponsorS-can

Description

While doing #19044 I tried to sort all the GCC warnings to see if we missed any that we could add. I came up with a list that we could add without triggering any new warnings (for me), and a list of ones tthat maybe looked good, but which do trigger today. I'm thinking that we can add these with little to no trouble:

  -Wunused-but-set-parameter -Wunused -Wuninitialized -Wstrict-aliasing=5
  -Wswitch-bool -Wtrampolines -Wunused-but-set-variable -Wvariadic-macros
  -Wsync-nand -Wdate-time -Wsizeof-array-argument
  -Wunused-parameter -Wunused-local-typedefs 
  -Wshift-count-negative -Wshift-count-overflow
  -Wc99-c11-compat -Wcast-align -Wpacked
  -Wmissing-format-attribute -Wsuggest-attribute=noreturn -Wsuggest-attribute=format

And we can add these GCC-6 only options with little to no trouble:

  -Wshift-negative-value -Wshift-overflow=2 -Wignored-attributes

These warnings look useful but they trigger on our current code. Is it worth fixing these warnings?

 -Wconversion
 -Wsign-conversion
 -Wunsafe-loop-optimizations
 -Waggregate-return // one place only, I believe.
 -Wdouble-promotion // Pretty easy to fix.
 -Wunsuffixed-float-constants // We have these all over.
 -Wcast-qual
 -Wstrict-overflow=n
 -Wstrict-overflow
 -Wpadded
 -Wjump-misses-init // lots of these; are any bugs?
 -Wswitch-default
 -Wsuggest-attribute=pure
 -Wsuggest-attribute=const
 -Wfloat-conversion   // just a dozen or so.
 -Woverlength-strings // only in tests

And these gcc6-only warnings trigger on our current code:

   -Wnull-dereference
   -Wunused-const-variable
   -Wduplicated-cond

Child Tickets

TicketStatusOwnerSummaryComponent
#19216closednickmOur approach to warnings leaves too few warnings with Clang.Core Tor/Tor

Attachments (1)

warnings.html (15.0 KB) - added by nickm 3 years ago.
warnings categorized

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by nickm

Attachment: warnings.html added

warnings categorized

comment:1 Changed 3 years ago by nickm

I've attached a table where I tried to sort all the warnings in gcc6.1. There are some "writeme" elements left over. I'll post updated versions at https://people.torproject.org/~nickm/warnings.html

comment:2 Changed 3 years ago by nickm

I've made a push_pop_warnings branch to use gcc/clang facilities to temporarily disable a warning for a single function or header, plus an example of how to use it. This might be handy to anybody who wants to take on this ticket.

comment:3 Changed 3 years ago by nickm

Also: if you are thinking of working on this ticket, remember that not every GCC version has every warning. You can find the historical set of GCC manuals here: https://gcc.gnu.org/onlinedocs/ .

comment:4 Changed 3 years ago by nickm

actually, git and grep were a better solution for "what was introduced when".

comment:5 Changed 3 years ago by nickm

Branch bug19180_easy now has the easy cases from https://people.torproject.org/~nickm/warnings.html . The remaining "tricky" cases are maybe worth fixing so they don't trigger on our code, and maybe not. I've annotated the table with which GCC version introduced each one. (The "GCC 3" annotations mean "GCC 3 or earlier".)

comment:6 Changed 3 years ago by nickm

Status: newneeds_review

Okay. bug19180_easy now has all of the easy fixes, including the "tricky" ones that were pretty easy to fix. I'll write up a synopsis of the remaining tricky ones before too long.

comment:7 Changed 3 years ago by bugzilla

FWIW, Tor Browser is compiled with

-Wdeclaration-after-statement
-Wpointer-to-int-cast
-Wcast-align

(The "GCC 3-" annotations mean "GCC 3 or earlier" ;)

comment:8 Changed 3 years ago by cypherpunks

The following change in 54610f721d66c41280e64674c27869eca381f059 got caught in the float suffix removal. The tests are not affected by it, but IMO it's still an erroneous change.

diff --git a/src/test/test_channeltls.c b/src/test/test_channeltls.c
index f5fa50c..3a73c77 100644
--- a/src/test/test_channeltls.c
+++ b/src/test/test_channeltls.c
@@ -51,7 +51,7 @@ test_channeltls_create(void *arg)
   channel_t *ch = NULL;
   const char test_digest[DIGEST_LEN] = {
     0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a,
-    0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14 };
+    0x0b, 0x0c, 0x0d, 0x0e, 0x0, 0x10, 0x11, 0x12, 0x13, 0x14 };
 
   (void)arg;
 
@@ -97,7 +97,7 @@ test_channeltls_num_bytes_queued(void *arg)
   channel_t *ch = NULL;
   const char test_digest[DIGEST_LEN] = {
     0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a,
-    0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14 };
+    0x0b, 0x0c, 0x0d, 0x0e, 0x0, 0x10, 0x11, 0x12, 0x13, 0x14 };
   channel_tls_t *tlschan = NULL;
   size_t len;
   int fake_outbuf = 0, n;
@@ -184,7 +184,7 @@ test_channeltls_overhead_estimate(void *arg)
   channel_t *ch = NULL;
   const char test_digest[DIGEST_LEN] = {
     0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a,
-    0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14 };
+    0x0b, 0x0c, 0x0d, 0x0e, 0x0, 0x10, 0x11, 0x12, 0x13, 0x14 };
   double r;
   channel_tls_t *tlschan = NULL;

comment:9 Changed 3 years ago by nickm

Added a fixup for that; thanks!

comment:10 Changed 3 years ago by nickm

ok, I really think I'm done for now. I've gone over the easy and not-too-hard cases. Of the warnings I originally considered, they are either Out, Done, or not-yet-done.

Here are the ones that I thought we might want to do where I decided against them:

  suggest-attrubte=const (4.6)
  suggest-attribute=pure (4.6)

    Rationale: these just suggest attributes that the compiler can use
    to optimize code better (if we get them right) but which will make
    the code incorrect (if we're wrong).

  jump-misses-init (4.6)

    Rationale: triggers all over.  In many (all?) cases, the missing
    initializer is for a variable that is not used after the jump. Our
    existing static analysis tools SHOULD catch the cases where we can
    _use_ an uninitialized variable.

  unsuffixed-float-constants (4.6)
    Not even like a bug; 1.0 is a fine way to spell the (double) 1.0.

  strict-aliasing=5??? (3)
    Requires strict-aliasing, which we disable.

  disabled-macro-expansion (clang)
    Triggers in stdio.h

  extended-offsetof (clang)
    We require this extension.

  used-but-marked-unused (clang)
    We need to be able to use "unused" to mean "maybe unused".

Here are the ones that we might want to look at later:

  cast-qual (4.6)

    Rationale: triggers everywhere, even in some pretty normal C.  Would
    be nice to have it trigger less, but would need to blow up a bunch
    of API things.  Bigger project.

  conversion (4.6)

    Rationale: triggers all over.  Probably wrong code in some
    cases, but careful thought needed in most Bigger project.

  sign-conversion (4.6)

    Triggers ALL OVER.  Quite possibly a bug in some cases, though.
    Bigger project.

  cast-align (3)
    We already do this safely. Need to re-test on a system with
    stronger-than-intel alignment rules, though.

  shadow (3)
    mistake; worth fixing.

  switch-default (3)
   Not sure this is a good idea; somet of these look like mistakes,
   but some don't.

  assign-enum (clang)
   triggers all over; worth fixing.

  conditional-uninitialized (clang)
   triggers all over; not sure whether this is worth fixing.

These should not become on-by-default, but they're worth hand-auditing

  strict-overflow=3...5 (4.2)

    Behaves pretty differently on different GCC versions.

    We get warnings for just about every case where we have pointer
    math in an addition. That seems nutty.

  padded (3)
    Not a mistake.  Worth looking over for hand-audit purposes, but mostly
    harmless.

  unsafe-loop-optimizations (4.1)
    Worth hand-auditing, but triggers on every kind of interesting for loop.

  covered-switch-default
    Usually this is defensive programming, but it could be a mistake
    in some cases, or could cover up future mistakes?

Aaand the bug19180_easy branch is still needs_review. :)

comment:11 Changed 3 years ago by andrea

Status: needs_reviewmerge_ready

Begin review of nickm's bug19180_easy branch:

da00395318ef0d762b0d9affb494101a72f3b3ab:

  • This looks fine, but I haven't got a gcc 6 handy to test with; I presume that testing for this branch including making sure the code builds cleanly with new warnings enabled?

(I see that other commits in this series also include fixes to make the
code clean under the relevant warnings, so I suppose that's a yes)

9bba1707c62170a89af8becbdda6c6e47be215a3:

  • This looks okay to me

22b7bdd670cfeba6c5afcb6e177d27de45943ea6:

  • This looks good

54610f721d66c41280e64674c27869eca381f059:

  • This looks fine to me given the subsequent fixup in bf1d9c725cb45df1eb7b0a4be57ae9da36763432

4d9c67ff2e202ff9bcc171701cf73527e833f0ed:

  • Strong agreement about huge string constants in the unit tests; there's lots of places where not having them would make the unit tests much hairier; we'd end up either having to read them from a file or construct them at run time from smaller string constants.
  • Is there any way to make enabling/disabling the warning contingent on its existence rather than on the gcc version? This seems really brittle if we ever change which gcc versions a particular warning is enabled by default for.

94fb4f94305dd725176c99b7276fc8cbb133865e:

  • Looks good, and like it also fixes a bug setting have_gcc5 = no rather than have_gcc6 = no.

bf1d9c725cb45df1eb7b0a4be57ae9da36763432:

  • Looks good to me

a74f3abc52ded6cb69b9272d8dac3f22528bd98d:

  • The commit message suggests this addresses the point I raised in regard to 4d9c67ff2e202ff9bcc171701cf73527e833f0ed, but it actually only has a changes file.

d979da8b017989e4aa053396b4ca37bd7deb980d:

  • This actually does what a74f3abc52ded6cb69b9272d8dac3f22528bd98d says; looks good

1ee7cd459257bcc2cce614a65cd861f5ff8ea5aa:

  • Good catch

7912aff13b464e229eeacbe5805356a4ffa2d66a:

  • Are we worried about the possibility of something like clang and gcc adding the same warning name for different behavior?

2ab4bb35feafb1ef0aab9cb55e8201ce31ab9a1c:

  • Looks okay to me.

ae82061f62bb7b9558ce81edb3271e000931b188:

  • Looks good

aea2e38cea967415ec170dd5c5aacef02c113bce:

  • That's quite a diff. I think it's all fine though.

7f8c6723ba9c66aae8a8c64e8fa2f21a3e860ff3:

  • This looks okay but does -Wstring-conversion get us anything but syntactic contortions like ((answer) != NULL) ?

End nickm/bug19180_easy code review; I think this is probably okay to merge.

comment:12 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

comment:13 Changed 3 years ago by nickm

I've merged bug19180_easy to master. I'll make new tickets for the remaining warnings listed above.

I do expect that we'll run into at least one surprising warning when Jenkins or some unsuspecting users tries to build with some compiler version I didn't get to try. We'll fix those as they turn up.

comment:14 Changed 3 years ago by nickm

Actual Points: 4
Points: 4
Resolution: implemented
Sponsor: SponsorS-can
Status: merge_readyclosed

New tickets: #19378 , #19379 , #19380. Tagging them all as 'gcc-warnings'. Closing this.

Note: See TracTickets for help on using tickets.