Fix compilation warnings with clang 10.0.0

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 (moved) for the GCC version of this)

changed milestone to %Tor: 0.4.4.x-final

Trac:
Child Ticket(s): #34131 (moved), #32579 (moved)

added actualpoints::1 component::core tor/tor milestone::Tor: 0.4.4.x-final owner::nickm priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
warnings.txt

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.

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

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 (moved) for the last time we talked about clang -Wimplicit-fallthrough.

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.

Trac:
Status: new to accepted
Owner: N/A to 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.

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.

https://infektor.net/posts/2017-01-19-using-cpp17-attributes-today.html#using-the-fallthrough-attribute

I don't think __has_cpp_attribute works in C?

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:

• bug34078_035 -- PR at https://github.com/torproject/tor/pull/1882
• bug34078_master -- PR at https://github.com/torproject/tor/pull/1883

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.

Trac:
Actualpoints: N/A to 1
Status: accepted to needs_review

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.

Trac:
Status: needs_review to merge_ready

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

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

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

added 8h of time spent

mentioned in issue #34131 (moved)

mentioned in issue #34233 (moved)

mentioned in issue #32579 (moved)

moved to tpo/core/tor#34078 (closed)

mentioned in issue tpo/core/tor#34233 (closed)