-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add simple static checker based on clang-query #833
Add simple static checker based on clang-query #833
Conversation
b603548
to
3b05077
Compare
Here's another interesting one: Unfortunately we use quite a few divisions, so this does not lead anywhere. We could also check easily for reserved identifiers. More suggestions? |
grml, apparently clang-query uses a |
Is there any way to tell if the divisor is a compile time constant? We generally don't want divisions ending up in the machine code (though if its in some setup routine it's not that big a deal) as they're non-constant time[1] and slow (e.g. 50-100 cycles on x86, worse on a lot of arm). ... but divisions with constants get converted by the compiler to multiples via strength reduction. I believe the codebase is free or nearly free of divisions with non-constant divisors (maybe there is one in scratch space sizing or something like that). I've failed to yet fully impress my practice of always using masking over division onto Pieter yet... but there are some cases where masking doesn't easily work. [1] also, stock valgrind ctime check will not catch divisions involving secret data. I have a pretty trivial patch to valgrind on my laptop so that it does... and it passed for me as of the last time I ran it. I'm not really eager to figure out how to make travis use a custom compiled copy of valgrind... |
3b05077
to
fb24278
Compare
Right, for example.
This is the reference for the matcher: You can match on "constantExpr" but this is not what we want. This matches constant expression as seen from the point of view of the AST, i.e., this matches places where only constant expressions are allowed (such as after This technique in general does not enable us to get insights on what the compiler will do with the code. It's really just pattern matching on the AST, so it's merely a clever grep. (One can do more advanced things with clang tooling but this requires more effort than writing matching expressions.) What we could do is to check if the divisor is an integer literal, but even this is not precise we have also a few |
931cc9b
to
3ecd681
Compare
This uses https://github.com/rizsotto/Bear, which intercepts make, to generate a JSON compilation database (see https://clang.llvm.org/docs/JSONCompilationDatabase.html). Bear is nice because we can easily install it on Travis. There are other tools that may work better when used locally, e.g., https://github.com/nickdiego/compiledb . |
0858d47
to
eed297b
Compare
Ready for review. Travis fails but this is intentionally. See it in action here: https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/736511002#L1254 (complains about Edit: argh, the links to the exact lines don't work due to the output folding. |
This is pretty cool. I'll have to play with it a bit. |
eed297b
to
0114113
Compare
I added a long commit message and updated the PR description to this message. |
As identified in bitcoin-core#829 and bitcoin-core#833. Fixes bitcoin-core#829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent.
This add a simple static checker based on clang-query, which is a tool that could be described as a "clever grep" for abstract syntax trees (ASTs). As an initial proof of usefulness, this commit adds these checks: - No uses of floating point types. - No use of certain reserved identifiers (e.g., "mem(...)", bitcoin-core#829). - No use of memcmp (bitcoin-core#823). The checks are easily extensible. The main purpose is to run the checker on CI, and this commit adds the checker to the Travis CI script. This currently requires clang-query version at least 10. (However, it's not required not compile with clang version 10, or with clang at all. Just the compiler flags must be compatible with clang.) Clang-query simply uses the clang compiler as a backend for generating the AST. In order to determine the compile in which the code is supposed to be compiled (e.g., compiler flags such as -D defines and include paths), it reads a compilation database in JSON format. There are multiple ways to generate this database. The easiest way to obtain such a database is to use a tool that intercepts the make process and build the database. On Travis CI, we currently use "bear" for this purpose. It's a natural choice because there is an Ubuntu package for it. If you want to run this locally, bear is a good choice but other tools such as compiledb (Python) are available.
0114113
to
4831f2e
Compare
FWIW
|
@elichai Yeah, where did you get this list from? Is this really all about all names? I thought |
https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html About
But on 7.1.3 "Reserved identifies" it says:
This paper proposing to change that also agrees on that interpretation: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2572.pdf (See "Future Library Directions")
EDIT: I realize now that you asked about identifier vs macro, and not about if it's related to |
Standard on 6.8.3 Semantics:
and also Aaron Ballman(the author of that paper, and part of WG14 committee) told me:
So yeah :/ |
It makes sense: if some macro is going to get called E1234 then you obviously can't have a variable named E1234 without trouble. |
…ify_t 1f4dd03 Typedef (u)int128_t only when they're not provided by the compiler (Tim Ruffing) e89278f Don't use reserved identifiers memczero and benchmark_verify_t (Tim Ruffing) Pull request description: As identified in #829 and #833. Fixes #829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent. This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833. ACKs for top commit: sipa: utACK 1f4dd03 jonasnick: ACK 1f4dd03 Tree-SHA512: c0ec92798f3c94f3ef6ac69b3f0f39a39257a32be9d9a068832cece1ebe64c89848b70e44652fc397004b8b240883ac4bc0c8f95abbe4ba4b028de120e6734bf
This warns when we define macros with some reserved identifiers, e.g., with underscores. It does not catch all reserved identifiers though.
Hm, closing for now... clang-query is a neat idea, but I'm not convinced that the checks introduced here (namely mostly "reserved identifier" stuff) are worth maintaining this, in particular because clang-query is a bit finicky. This conclusion could certainly change if we want to use something like this here for tracking magnitude (#1001) or other advanced stuff. And then, we may also want to look into other similar tools such as CodeQL. |
This add a simple static checker based on clang-query, which is a
tool that could be described as a "clever grep" for abstract syntax
trees (ASTs).
As an initial proof of usefulness, this commit adds these checks:
The checks are easily extensible.
The main purpose is to run the checker on CI, and this commit adds
the checker to the Travis CI script.
This currently requires clang-query version at least 10. (However,
it's not required not compile with clang version 10, or with clang
at all. Just the compiler flags must be compatible with clang.)
Clang-query simply uses the clang compiler as a backend for
generating the AST.
In order to determine the compile in which the code is supposed to be
compiled (e.g., compiler flags such as -D defines and include paths),
it reads a compilation database in JSON format. There are multiple ways
to generate this database. The easiest way to obtain such a database is
to use a tool that intercepts the make process and build the database.
On Travis CI, we currently use "bear" for this purpose. It's a natural
choice because there is an Ubuntu package for it. If you want to run
this locally, bear is a good choice but other tools such as compiledb
(Python) are available.