Adding clang-tidy checks to OpenSSL

Generally speaking, I'd be in favor of something like this, though I think we need to be very careful here how we go about it.

A few notes:

1) clang-tidy doesn't understand our template files (i.e. our perl-ified .in and .inc files). As such it seems we need to be careful to only run this linting on built trees, so we don't run afoul of any diagnostic errors for type definitions we define in generated headers.

2) We will want to be very careful about what lint checks we are willing to enable. for instance, running with only the concurrency-mt-unsafe check on my local tree reports about 150 errors indicating usafe mutithread usage, mostly centering around the getenv, strerror, and exit functions, all of which are defined in posix as MT-SAFE, so there some question in my mind as to what clang-tidy is trying to get us to do there, and what our expectations are.

3) For checks we are willing to enable, we should ensure that any enablement is only done after a run of tidy produces no errors/warnings against that check. Some of these even trivial checks above produce 1000's of errors, and I would hate to enable something like this only to have it ignored because its too much of a lift for people with follow on patches to have to clean up the mess.

I'd be in favor of doing something like:

1) Create a ci job to run clang-tidy on the source tree as part of a pull request

2) Check in a .clang-tidy file that enables 0 checks (i.e. the trivial case)

3) Create a long running issue for anyone interested to scrub the tree of various errors and enable the corresponding check.

It might make for a series of "good first issue", work efforts to expose people to the source tree, but doing broad based tree-wide cleanups.

For checks we are willing to enable, we should ensure that any enablement is only done after a run of tidy produces no errors/warnings against that check. 

I've seen mixed results with this philosophy on a very large monorepo. Very frequently resolving all the existing violations turns into a non-trivial exercise and / or results in significant churn in legacy areas. Perhaps OpenSSL is not so large a codebase that this effect is as pronounced.

It may come down to the question of whether there is a view that any particular check / policy is of significantly greater value to prevent future violations of than it is to resolve existing violations of. Those tend to be the cases where you end up stuck in a sort of purgatory where it's not worth the bother to fix every single one of the existing violations and then more violations keep slowly creeping in (because it turns out human reviewers can't achieve 100% consistency in terms of rule / policy enforcement)... which over time just keeps strengthening the argument that it's not worth the bother, even when there's broad consensus that it would be nice to prevent future violations.

@Jeremy Walch 

I had a look at CodeChecker https://github.com/Ericsson/codechecker which can act as a frontend and provide a database of violations which can be used to “ignore” legacy violations.

Do you have experience with this tool? I think it sounds promising because it supports a variety of checkers including Clang Static Analyzer and cppcheck.

@Frederik Wedel-Heinen 

I can't say I do.

It may be worth noting that clang-format should by itself support in-line annotations like // NOLINT and similarly-spirited friends.

Another thing to note is that some of the clang-tidy checks do have a "fix suggestion" feature. (They aren't AI-generated slop; the fix logic is explicitly encoded by the check author.) From my experience, a lot of them were like modernize-* ones in C++ (eg, converting an STL iterator loop to a range-for), which obviously would not be applicable for this project. I tend to suspect that a lot of the checks this project may find useful will not have auto-fix transformations, but I mention it for completeness with respect to the conversation with respect to fixing existing violations.

Anyway, I suspect the question of what exactly the mechanism used to selectively permit violations is matters far less than the conversation of whether or not it ought to be used at all or under which conditions.

The failure mode we'd see with allowing violations in a larger codebase was that some violations would never really go away over time or even see a net increase in violations.

The lack of commitment to follow through on fixing existing violations of some type really bothered some people. The argument was that if it was a policy worth enforcing on new code then surely it was worth eventually also cleaning up the old code (with peripheral sentiments around that like legacy code continuing to serve as a bad example). I am less convinced of this. There's tons of anti-patterns / bad practices that clang-tidy can detect that aren't guaranteed to actually result in functional bugs. With legacy code that has limited maintenance resourcing it's actually better to only fix things that are believed to be impactful in the functional sense. I think it's fair to say that at a bare minimum existing violations should have a committed timeline on at least being audited for potential impact, however. (As opposed to just getting a blank check when the check is enabled and never getting seriously looked at.) Anyway, this aspect of "grandfathering" in violations is something that the project community would need to consider if it wants to go down that road.

Another thing that bothered some people was that occasionally some change would add new violations in the strict detection mechanism sense, but they weren't new "in spirit" (stuff originating in headers, stuff that is a consequence of preexisting type-width / mismatch issues, etc.). Some of this was a technical limitation of the linter we were using being relatively stupid compared to clang-tidy (especially in terms of where it flagged things), but even with clang-tidy it can happen sometimes. Then gatekeepers / reviewers would refuse to accept the change under the auspices of the violation mechanism being intended to be a "ratchet" (only goes down). Again, it's more of social / political problem than a technical one as such, but it's one that has to be considered if you want to allow violations at all.

Here's a PR of your proposed first step :) https://github.com/openssl/openssl/pull/29580