Opened 4 weeks ago

Closed 3 weeks ago

#34078 closed defect (fixed)

Fix compilation warnings with clang 10.0.0

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: backport-035, backport-041, backport-042, backport-043 Actual Points: 1
Parent ID: Points:
Reviewer: Sponsor:

Description

Fedora 32 includes Clang 10.0.0, which is a bit more strict about switch-statement fall-through annotations, and about converting enums to bools. This causes us to hit compilation warnings on all supported Tor versions.

(See also #34077 for the GCC version of this)

Child Tickets

TicketStatusOwnerSummaryComponent
#32579closedUse clang's -Wimplicit-fallthrough and fallthrough attribute on case statementsCore Tor/Tor
#34131closednickmFix logic error in parse_extended_hostnameCore Tor/Tor

Attachments (1)

warnings.txt (38.1 KB) - added by nickm 4 weeks ago.

Download all attachments as: .zip

Change History (14)

Changed 4 weeks ago by nickm

Attachment: warnings.txt added

comment:1 Changed 4 weeks ago by nickm

I'm not sure whether we should clean all of the switch-statement fall-through warnings or not. There are a whole bunch of them. (See attachment.)

There is just one convert-to-bool warnings though:

src/core/or/connection_edge.c:1662:40: error: converting the enum constant to a boolean [-Werror,-Wint-in-bool-context]
      (*type_out == (ONION_V2_HOSTNAME || ONION_V3_HOSTNAME) ? "onion " : ""),

That looks like a real bug.

comment:2 Changed 4 weeks ago by cypherpunks

I'm not sure whether we should clean all of the switch-statement fall-through warnings or not. There are a whole bunch of them. (See attachment.)

http://lkml.iu.edu/hypermail/linux/kernel/1905.0/04622.html

comment:3 in reply to:  1 Changed 4 weeks ago by teor

Replying to nickm:

I'm not sure whether we should clean all of the switch-statement fall-through warnings or not. There are a whole bunch of them. (See attachment.)

See #32579 for the last time we talked about clang -Wimplicit-fallthrough.

comment:4 Changed 3 weeks ago by nickm

Looking at the code more: it seems that clang actually does catch the same cases here as gcc does, but that clang does not recognize the same magic "/* fall through" comments as gcc.

Current plan is to disable the warning in 0.3.5 through 0.4.2, but replace the fallthrough with an attribute in 043 and later.

comment:5 Changed 3 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:6 Changed 3 weeks ago by nickm

Okay, I have a proof-of-concept as https://github.com/torproject/tor/pull/1882 against maint-0.3.5. I don't suggest merging it yet: it needs a changes file. I've just made a PR so that we can see what CI thinks.

comment:7 Changed 3 weeks ago by nickm

Once CI passes, I'll add a changes file, and then remove the final (automated) commit from the branch. At that point, I think the best move is to merge the non-automated parts of the branch to all supported versions, and then apply the automated part individually to each version. That way we will get the right fix everywhere without any conflicts.

comment:9 Changed 3 weeks ago by nickm

I don't think __has_cpp_attribute works in C?

comment:10 Changed 3 weeks ago by nickm

Actual Points: 1
Status: acceptedneeds_review

Okay, I've made some "prelim[inary]" branches that do everything except the bulk-replacement. They are:

  • bug34078_prelim_035
  • bug34078_prelim_041 -- this is the only one that had a merge conflict
  • bug34078_prelim_042
  • bug34079_prelim_043

Then I made a script to do the commit with the actual fix. Two examples of its use are:

If these pass review, my plan is that we should merge the "prelim" branches as normal, and then one by one apply the fix script to each maint branch, then do an "ours" merge to the next branch before applying the script to that branch too. In this way, we don't need to worry about conflicts from code movement.

comment:11 Changed 3 weeks ago by ahf

Status: needs_reviewmerge_ready

The preliminary patches looks good. In the master example PR, the comment /* Fallthrough is to disallow since this means the bucket has reached 0. */ is erased in hs_dos.c. I think that is the only issue I could spot.

I don't have Clang 10 locally to try it though.

If the comment is restored, I think this can be merged.

comment:12 Changed 3 weeks ago by nickm

I think that's also causing a spurious failure in master. I'll fix it, validate CI, and merge.

comment:13 Changed 3 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay, fixed all the little subcases, opened #34131 for the bool issue and fixed it. Merging to 0.3.5 and forward.

Note: See TracTickets for help on using tickets.