Opened 5 years ago

Closed 5 years ago

#13577 closed defect (fixed)

Fix clang compilation warnings in tor, trunnel

Reported by: teor Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: clang
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by teor)

My branch silence-warnings on github contains fixes to silence clang warnings under --enable-expensive-hardening, including:

+ implicit truncation of 64 bit values to 32 bit;
+ const char assignment to self;
+ tautological compare; and
+ additional parentheses around equality tests. (gcc uses these to

silence assignment, so clang warns when they're present in an
equality test. But we need to use extra parentheses in macros to
isolate them from other code).

Branch: silence-warnings
Repository: ​​​​​​

Nick, do you want the trunnel patch submitted upstream?

Child Tickets

Change History (7)

comment:1 Changed 5 years ago by teor

Description: modified (diff)
Status: newneeds_review

I obviously need to check copy-paste before I hit submit.

comment:2 Changed 5 years ago by nickm

Yes, please send me the trunnel thing as a patch on trunnel.

Other than that, I like all of this except for the part where we liter the code with !!. That clang warning doesn't seem to me like it's actually making our code any cleaner; it's making it more confusing. Can we move those !!s into the macros in question, or turn the macros into inline functions, or something like that? Alternatively, I wouldn't mind -Wno-parenthesis-equality.

comment:3 Changed 5 years ago by teor

Yes, !! is pretty ugly, isn't it?

I think -Wno-parenthesis-equality is the best option, as it's obviously not adding that much value if it's causing these sorts of false positives. But I'm concerned that it is actually adding value in some cases (i.e. = vs == in conditionals)

I also have doubts about moving the !! into the macro, because !! would have to sit outside the macro's outer brackets to be effective. And that causes its own issues.

We could use inline functions, but that might break some of the consistency / encapsulation / optimisation. I'll look into it.

comment:4 Changed 5 years ago by teor

It turns out that only macros like (A == B) trigger the error.
Adding a no-op like "| | 0" to the end of the macro breaks this pattern-matching.
The final transformation is: (A == B) to ((A == B) | | 0) with an appropriate comment.

The benefit of this change over the "!!" change is that it can be performed in the macro itself. (Perhaps this also works for "!!" in the macro? I can't remember if I checked it or not. But I think "| | 0" is just as clear as "!!".)

The fixed branch is: silence-warnings-in-macros
It includes the trunnel and non-trunnel commit.
It also includes a very minor fix to a comment in src/or/or.h:

 #define PROXY_SOCKS5 3
-/* !!!! If there is ever a PROXY_* type over 2, we must grow the proxy_type
+/* !!!! If there is ever a PROXY_* type over 3, we must grow the proxy_type
  * field in or_connection_t */

Normally, I'd put this in its own commit and bug, but it hardly seems worth it. (It snuck in accidentally because I fixed the typo while doing the other changes.)

comment:5 Changed 5 years ago by nickm

Silly question: do macros like (rvalue == lvalue) also trigger this, or are only '(lvalue == rvalue)' macros affected?

comment:6 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:7 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I've applied the patches to master and trunnel, then re-run trunnel to get the change properly. Thanks!

Note: See TracTickets for help on using tickets.