Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2696 closed defect (implemented)

Tor doesn't compile under LLVM/clang with --enable-gcc-warnings

Reported by: sjmurdoch Owned by:
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A Tor default build builds cleanly under LLVM/clang (since bug #2689 was fixed). However, it does not build cleanly with --enable-gcc-warnings set during ./configure. This is for two reasons:

1) configure sets CFLAGS to include -Wnormalized=id -Woverride-init, which do not exist in clang 2.9

2) -Wshorten-64-to-32 is set, which triggers a warning in geoip.c

geoip.c:437:33: warning: implicit conversion loses integer precision: 'long' to 'unsigned int' [-Wshorten-64-to-32]

ent->last_seen_in_minutes = now / 60;

geoip.c:441:33: warning: implicit conversion loses integer precision: 'long' to 'unsigned int' [-Wshorten-64-to-32]

ent->last_seen_in_minutes = now / 60;

I've attached a patch which fixes (1). If clang is being used, and the version is <= 2.9, -Wnormalized=id -Woverride-init are not set. This logic is so that if later versions of clang support these options, we won't disable them for all time without noticing.

I'm not sure if (2) is considered a bug or not. I suppose it will do the wrong thing when time_t is negative, but that was a long time ago.

This is using Tor from Git master (26009a3) on MacOS X 10.6.6.

Child Tickets

Attachments (1)

0001-Fix-compilation-under-LLVM-clang-with-enable-gcc-war.patch (2.2 KB) - added by sjmurdoch 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by nickm

(2) will also give us trouble if anybody is still using 32-bit-int platforms in the year 6053 CE. We might as well add an explicit cast so that we build without warnings there, though.

comment:2 Changed 9 years ago by Sebastian

Status: newneeds_review

Sounds like we should do this on 0.2.2.x. Branch bug2696 in my repository does that. It backports your patch and changes whitespace at end of line issues. I hope it is ok that I kept you as author of the commit.

comment:3 Changed 9 years ago by Sebastian

Milestone: Tor: 0.2.2.x-final

comment:4 Changed 9 years ago by sjmurdoch

Sebastian, looks fine to me. Thanks.

comment:5 Changed 9 years ago by arma

I'm delegating review on this one to nickm -- I know how much he enjoys reading autoconf diffs, and I wouldn't want to deprive him of it.

comment:6 Changed 9 years ago by nickm

The autoconf thing looks fine.

The MAX_LAST_SEEN_IN_MINUTES thing seems to involve a signed/unsigned comparison though. If "now" is negative, we'll hit the assertion. It also seems to be assuming 32-bit int; the more proper way to get a maximum here woudl be to define MAX_LAST_SEEN_IN_MINUTES in terms of UINT_MAX. Do we care about these?

(And by 0x3F...u? Why not, say, 0x4F...u) or 0x5F...u?

comment:7 in reply to:  6 Changed 9 years ago by Sebastian

Replying to nickm:

The autoconf thing looks fine.

The MAX_LAST_SEEN_IN_MINUTES thing seems to involve a signed/unsigned comparison though. If "now" is negative, we'll hit the assertion. It also seems to be assuming 32-bit int; the more proper way to get a maximum here woudl be to define MAX_LAST_SEEN_IN_MINUTES in terms of UINT_MAX. Do we care about these?

hrm. Does our other code work where int isn't 32bit? I thought we didn't, but I could well be wrong. How could negative now happen? Should we have a wrapper around time() that prevents negative values (and later implements the configurable time offset if we choose to do that)?

(And by 0x3F...u? Why not, say, 0x4F...u) or 0x5F...u?

Because that's the largest value you can store in last_seen_in_minutes

comment:8 Changed 9 years ago by nickm

Ah, I had totally missed that last_seen_in_minutes was a bitfield. That makes sense now. I'll add a doxygen comment to MAX_LAST_SEEN_IN_MINUTES to explain the value.

time_t is allowed to be negative if it's before 1970. It generally isn't, but it's nice to avoid signed/unsigned comparisons.

Tor works if int is 64-bit. At least, Tor is supposed to work if int is 64-bit.

Will add that comment and merge.

comment:9 Changed 9 years ago by nickm

I tweaked some stuff in branch bug2696 in my repository; Sebastian says that there are remaining clang errors he plans to fix.

comment:10 Changed 9 years ago by Sebastian

The remaining issue is this:

clang: warning: argument unused during compilation: '-O2'

this is because we duplicate the -O2 argument:

/opt/local/bin/clang  -g -O2 -Wall -g -O2 ...

I'm not sure how to solve this, because I don't know where the first -g -O2 comes from.

comment:11 Changed 9 years ago by nickm

I'm guessing that autoconf adds it by default? If so we can just remove ours.

comment:12 Changed 9 years ago by Sebastian

Yes, I confirmed that it gets added by default. Branch bug2696 for an updated version.

comment:13 Changed 9 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

looks fine; merging to maint-0.2.2 and master.

comment:14 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:15 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.