#27772 closed defect (fixed)

Compile without warnings on GCC 8.2.0 with LTO enabled

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

I propose that we should try to build without warnings on GCC 8.2.0 when the LTO option is enabled. (See #27728)

With LTO, the compiler generates some spurious warnings, but some warnings that reflect real cross-module issues that we should care about.

Child Tickets

Change History (9)

comment:1 Changed 23 months ago by nickm

Here are the warnings from src/app/tor itself:

  CCLD     src/app/tor
src/feature/nodelist/torcert.c: In function ‘tor_cert_sign_impl.constprop’:
src/feature/nodelist/torcert.c:55:19: warning: potential null pointer dereference [-Wnull-dereference]
   cert->exp_field = (uint32_t) CEIL_DIV(now + lifetime, 3600);
                   ^
src/feature/nodelist/torcert.c:56:23: warning: potential null pointer dereference [-Wnull-dereference]
   cert->cert_key_type = signed_key_type;
                       ^
src/feature/nodelist/torcert.c:54:19: warning: potential null pointer dereference [-Wnull-dereference]
   cert->cert_type = cert_type;
                   ^
src/lib/meminfo/meminfo.c: In function ‘tor_log_mallinfo’:
src/lib/meminfo/meminfo.c:48:8: warning: function call has aggregate value [-Waggregate-return]
   mi = mallinfo();
        ^
src/lib/crypt_ops/crypto_pwbox.c: In function ‘crypto_pwbox’:
src/trunnel/pwbox.c:161:56: warning: potential null pointer dereference [-Wnull-dereference]
                  &inp->skey_header.n_, inp->skey_header.elts_, newlen,
                                                        ^
src/core/mainloop/connection.c: In function ‘connection_free_minimal’:
src/core/mainloop/connection.c:665:12: warning: null pointer dereference [-Wnull-dereference]
       if (!CHANNEL_FINISHED(TLS_CHAN_TO_BASE(or_conn->chan))) {
            ^
src/feature/nodelist/routerparse.c: In function ‘router_parse_addr_policy.part.2’:
lto1: warning: function may return address of local variable [-Wreturn-local-addr]
src/feature/nodelist/routerparse.c:4406:17: note: declared here
   addr_policy_t newe;
                 ^
src/core/proto/proto_socks.c: In function ‘socks_request_set_socks5_error’:
src/trunnel/socks5.c:3075:16: warning: null pointer dereference [-Wnull-dereference]
   inp->version = val;
                ^
src/trunnel/socks5.c:3563:14: warning: null pointer dereference [-Wnull-dereference]
   inp->reply = val;
              ^
src/trunnel/socks5.c:3116:14: warning: null pointer dereference [-Wnull-dereference]
   inp->atype = val;
              ^
src/core/or/scheduler_kist.c: In function ‘update_socket_info_impl’:
src/core/or/scheduler_kist.c:198:5: warning: potential null pointer dereference [-Wnull-dereference]
     TO_CONN(BASE_CHAN_TO_TLS((channel_t *) ent->chan)->conn)->s;
     ^
src/core/proto/proto_socks.c: In function ‘fetch_from_buf_socks’:
src/trunnel/socks5.c:2690:16: warning: potential null pointer dereference [-Wnull-dereference]
   inp->version = val;
                ^
src/core/or/scheduler_kist.c: In function ‘channel_write_to_kernel’:
src/core/or/scheduler_kist.c:457:27: warning: potential null pointer dereference [-Wnull-dereference]
   connection_handle_write(TO_CONN(BASE_CHAN_TO_TLS(chan)->conn), 0);
                           ^
src/core/or/channelpadding.c: In function ‘channelpadding_send_enable_command’:
src/core/or/channelpadding.c:331:3: warning: potential null pointer dereference [-Wnull-dereference]
   tor_assert(BASE_CHAN_TO_TLS(chan)->conn->link_proto >=
   ^
src/core/or/channelpadding.c: In function ‘channelpadding_send_disable_command’:
src/core/or/channelpadding.c:299:3: warning: potential null pointer dereference [-Wnull-dereference]
   tor_assert(BASE_CHAN_TO_TLS(chan)->conn->link_proto >=
   ^
src/core/or/channel.c: In function ‘channel_rsa_id_group_set_badness.isra.10.part.11’:
src/core/or/channel.c:3436:54: warning: potential null pointer dereference [-Wnull-dereference]
     smartlist_add(or_conns, BASE_CHAN_TO_TLS(channel)->conn);
                                                      ^
src/lib/meminfo/meminfo.c: In function ‘signal_callback’:
src/lib/meminfo/meminfo.c:48:8: warning: function call has aggregate value [-Waggregate-return]
   mi = mallinfo();
        ^

comment:2 Changed 23 months ago by nickm

See my branch 'bug27772' with PR at https://github.com/torproject/tor/pull/356 .

I haven't written a changes for this yet, because I'd like a reviewer's opinion on how many of these to backport how far.

There are remaining warnings: I think there is some bug in GCC's LTO where it doesn't handle "#pragma diagnostic" properly -- since these are warnings we have explicitly suppressed in the code:

src/lib/meminfo/meminfo.c: In function ‘tor_log_mallinfo’:
src/lib/meminfo/meminfo.c:48:8: warning: function call has aggregate value [-Waggregate-return]
   mi = mallinfo();
        ^
src/test/test_bt_cl.c: In function ‘crash’:
src/test/test_bt_cl.c:43:24: warning: null pointer dereference [-Wnull-dereference]
     *(volatile int *)0 = 0;
                        ^

comment:3 Changed 23 months ago by nickm

I ported part of this to my bug25573-034-typefix branch in case we eventually merge #25573 to 0.3.4

comment:4 Changed 23 months ago by nickm

Status: assignedneeds_review

comment:5 Changed 23 months ago by dgoulet

Reviewer: ahf

comment:6 Changed 22 months ago by nickm

Priority: MediumHigh

comment:7 Changed 22 months ago by ahf

Status: needs_reviewmerge_ready

I think all of these commits can go in as they are generally cleaning up code.

I do however think that some of the comments that are added to suppress warnings from GCC will become confusing when you look at the code later on. For example: https://github.com/torproject/tor/pull/356/commits/620108ea7770608de72dcbea4ca73d6fb99c1109#diff-23701c678d2a4dce3ba19f90e4bf5b00R4440

The patches seems reasonably "small" to maybe backport all of them, or should we just backport the ones that causes compilation errors? Will any of our users who are using an older version of Tor be compiling Tor with such a new version of GCC?

comment:8 Changed 22 months ago by nickm

IMO anybody who wants LTO should be sticking with a fairly recent version. Looking over the commits again, I didn't see anything that looked like a scary bug in need of a backport... so if you didn't either, that should be fine.

I'll go over the comments to try to make them better.

comment:9 Changed 22 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay, I've squashed, fixed the comments, merged, fixed the conflicts. TY!

Note: See TracTickets for help on using tickets.