build2 / build2

build2 build system
MIT License
589 stars 15 forks source link

Support for static analysis operations (maybe by adding custom (meta-)operations?) #30

Open Klaim opened 5 years ago

Klaim commented 5 years ago

From Slack channel:

klaim [10:06 AM] Wondering how one would add static analysis done after build automatically. Maybe by adding a custom meta-operation?

Olivier Delannoy [12:01 PM] I wonder if this is not the custom build step that @boris is working on.

boris [1:03 PM] That's a good question. I think we first need to understand what's the desired workflow. For example, do we want it run implicitly after every rebuild (and maybe only on what's changed) or do we want it run only if requested explicitly (for example, because it's too slow), or both? Also, this will probably need to tie-in with the compilation database support. I think a GitHub issues for further discussion would be a good idea.

About static analysis

For example, do we want it run implicitly after every rebuild (and maybe only on what's changed) or do we want it run only if requested explicitly (for example, because it's too slow), or both?

In the project(s) I was thinking about the answer would be both:

  1. Execute static analysis automatically after something is compiled (not necessarily linked, no need to check other code).
  2. Execute static analysis independently on all the code or the code related to a target (with different level of dependencies, none, immediate, all).

This is to enable both developer's work station checks and ease adding static analysis to CI (though this last case is far easier to do with a CI script).

The main reason I can think about why static analysis would not be automatic is if it takes too long and that depends a lot on tooling and the code itself. Therefore I think it's important to give the choice. For example we have a situation where some components/projects are critical dependencies for all other projects in the company. Having them checked all the time (in diverse way) prevents a lot of issues that will only be visible once users try to use them, and often are silent issues. For these critical projects, having a maximum of checks going at all developments stages is useful. For the users projects, it's less important and can slow down development speed of things that are perceptible to the end-users (while critical projects are not perceptible, except when they fail).

It looks to me that if operation 1) is possible, then a choice of deciding if having reports from analysis should prevent further build operations would be useful (and relative to the project or use case).

How to enable this kind of feature: custom (meta-)operations?

Trying to look at the general picture, maybe having a way to add operations to the build system would enable static analysis operations. If the project using build2 can describe these operations in it's own source code, that would enable a lot of custom project or organization-specific operations without having to formalize an interface for every category of operation. One other advantage is that it would allow custom behaviors depending on some custom parameters, like stopping the build if static analysis report any important issues, or not, or generating some output locally or on a specific location online.

It would also go in the same direction that is being taken (work in progress at the moment if I understand correctly) to allow custom target definitions. I'm not sure how harder allowing custom operations would be compared to targets, but it looks like they would be less contrived in possible behavior than targets, which might be a design problem (in the same way enabling procedural scripts at build time enables a lot of problems).

One other possible usage of such custom operation would be to add a benchmark operation. This was slightly discussed before as already possible by running tests that are in a specific directory which contains only benchmarks. This is enough for most cases I believe. Now if instead of that we add a custom benchmark command, we can also add collection and storage in remote location for analysis to the whole process, after running the targets that are marked as being benchmarks. While this would be useful, it is to be determined if it's a big enough advantage to have that complex process being done through a custom build2 command, compared to just adding a separate script somewhere to do that, launched manually or by the benchmark tests programs themselves.

How to enable this kind of feature: custom target

If we were using GNU make, my understanding is that launching static analysis could simply be a target. In build2 it would be a target type, to generalize.\ In this case, automating analysis could be achieved by implementing a custom target and adding that target as requirement for projects that need to be analyzed throughly.

However, how then would we run analysis "on demand" and/or for a specific target? I didn't think much further yet, so maybe some target setup can allow this kind of things.

How to enable this kind of feature: specific analysis meta-operation

Static analysis is a well known topic for all languages so it's not something that is uncommon. Maybe just adding an operation (like for test) would be simpler and more useful. In this case, the main work would be to decide what are the general behaviours of static analysis and add a way to specify static analysis tools and usage patterns so that build2 can use them (in the same way it works with compilers).

boris-kolpackov commented 5 years ago

Thanks for starting the issue and capturing your thoughts. I think as a first step we need to get a good understanding of what are the common static analysis tools (which we may want to support in build2) and how they are typically (or atypically -- we may want to do things differently in build2, for example, to support incremental analysis) used.

We've done a bit of research on clang-tidy and it appears the way it is commonly used is by first generating the compilation database and then invoking a (Python) scripts that (presumably) executes the actual analyzer on each translation unit from said database. There is also a somewhat related discussion.

So I guess the question that I would like to answer is what other common static analyzers are there and how are they used? Also, I wonder if this "analyzing one translation unit in isolation" is the norm. Any tools analyze headers?

Klaim commented 5 years ago

Here is what I know on the subject, based on user experience, not on documentation, so it's just a quick preliminary overview:

Basically, almost all analysis tools I used so far through years (PVSStudio, VS's analyzer, clang-analyzer) requires that the code have been compiled or that the compilation database is provided, indeed. There is one exception where I don't remember: cppcheck.I don't know for Coverity (I've been in contact with their sellers but not with the actual tool).

That's why I assumed (and it is only an assumption based on user experience) that analysis would only happen after compilation (but not necessarily link).

For PVSStudio and VS's analyzer, I know (because I've used them a massive lot that way) that you can launch analysys on a source file (it have to be a translation unit), on a "project" (basically a target that result in a specific binary) or on a directory (or virtual directory in a Visual Studio solution).

CppCheck I never used on a single file but I did on a whole codebase. However, I don't remember that it was requiring any compilation step, so we'll have to check that.

Side Note: VS is progressively integrating clang tools with the rest of their tools/build-system, even if Microsoft already provide their own solution for the same problem.

One other thought: static analysis is used in all kinds of languages. Would support in build2 be language-independent? If so, research need to be wider to see if the initial assumptions (requiring a build, or not in interpreted languages? should it happen before intepretation/generating bytecode?) holds for all such tools. If we focus on C++ it would be simpler though.

Any tools analyze headers?

I don't remember any such tools being able to work on headers alone. Maybe cppcheck does, though it would be weird IMO.

boris-kolpackov commented 5 years ago

Thanks for the information. Do you know if PVSStudio, VS, etc., all work (or can work) off the (Clang) compilation database? Do they provide any way to invoke the analyzer directly on a specific translation unit (and with specific compile options)?

Regarding language-agnostics static analysis, to me this feels like very C/C++ language, or rather, C/C++ tooling-specific. I.e., the static analyzer operates on a translation unit (at least once you peel of things like the compilation database), it needs some compile options (-I, -D, etc), etc. And I am sure modules will make things even more interesting. So my first instinct is that this will probably be a functionality of the cc module, similar to the compilation database generation. Something like a post-compilation "hook" or some such (which feels like the most straightforward way to implement incremental static analysis). Here is a pertinent quote from the research I linked above:

Are we going to provide some integration for actually running the tools? In other words, one could have configured a build with static analysis tools which are automatically invoked either after update (we have a notion of post-operation which we can probably use here) or as a separate operation? Having static analysis diagnostics as part of normal compilation diagnostics definitely sounds appealing (though apparently they can take significantly longer than compilation plus there are false positives). If we go this route the question is then why do we even need the compilation database? Are these tools somehow rely on the ability to see all the commands at once or is it just the least invasive way to retrofit them into existing build systems?

@johan-boule Anything you can add on the Clang tooling, especially on invoking things directly rather than through compilation database/script?

Klaim commented 5 years ago

Do you know if PVSStudio, VS, etc., all work (or can work) off the (Clang) compilation database?

PVSStudio: it embarks clang in it's implementation details (among other things, last time I checked) so I suspect that yes it might rely on the database, maybe to augment it's own. But maybe it would be better to ask them directly? (I remember looking in the install directory and then asking if PVSStudio was basically clang-analyzer plus a few rules and they answered at the time that it was more like one of the source information they use to apply their rules)

VS analyzer: Last time I tried (last year) it was not (as far as I understand), but in the last version they integrated stuffs from clang so we'll have to check. Note that clang is provided as part of the VS install now (or is automatically detected when installed on the system through llvm's installer). I think they are open with this too so maybe discussing with them directly would help.

For others I don't know.

If we go this route the question is then why do we even need the compilation database? Are these tools somehow rely on the ability to see all the commands at once or is it just the least invasive way to retrofit them into existing build systems?

When using these tools, my assumption (just funded on the behavior I observed) is that it is easier to work with code that is already checked for compilation as the analysis tool can focus on other issues than, say, basic syntax errors, usage of unknown names etc. But that was just a guess.

I think we need more research and feedback from implementer to know about this.

I think it's reasonable to at least start with cc, though note that all widely dynamic languages have static analyzers too, mostly used by big companies. For example, Coverity alone supports;

C/C++, C#, Java, JavaScript, PHP, Python, ASP .NET, Objective-C, JSP, Node.js, Ruby, Andriod, Swift, and Fortran`

(source from 2 years ago: https://community.synopsys.com/s/question/0D53400003WcsSiCAJ/what-languages-are-supported-by-synopsys-static-analysis-coverity ) (their website is a bit more vague but there is at least half these languages, including Python, Ruby and Javascript).

I think it would not be a priority to support them all as static analysis can always be done separately. Maybe that can be defered when people decide to try adding other languages to build2.

Another related thing: please consider supporting (in the future?) more than one static analysis step/tools called sequentially. This is because are good enough to cover all the subtelties of c++ and even them (in their public marketing) recommend to use several static analysis tools (ideally). Iff it's not hard to plug in several ones as part of the static analysis step, it would be useful to enable this. In my experience, I always used at least 2 different static analysis tools to show me widely different concerns. Although supporting 1 is already a win, if it can be supported easily it would be more useful.

matta commented 2 years ago

I thought I would leave a few comments based on how I've seen static analyzers effectively used.

  1. I've never seen effective use of static analysis as part of a normal build process. It has always been a separately requested step. This is not to say this can't be done, and only half true (read on...), but too often static analysis is like "lint" and is too noisy to be useful during normal iterative development.
  2. Compiler warnings are a form of static analysis, which partially negates point (1). Some projects turn -Werror on and reject code with warnings. This leads to long debates about which warnings have enough value to enable, etc.
  3. clang-tidy is integrated with clang's LSP, and can surface clang-tidy warnings while editing. This needs their compilation database JSON to work. This works with header files, though requires a project convention that header files are "self contained" (i.e. a .cxx file that includes just that header file can compile).
  4. Point (3) raises the interesting question: which clang-tidy checks do you want enabled for day-to-day development and surfaced in your editor? The answer is usually the "high value" ones, which usually is "the ones that almost always point out a bug or coding convention that applies nearly universally."
  5. If a project has a class of "high value" clang tidy checks it stands to reason that occasionally it may be worth looking at the lower value ones, optionally. But incurring this cost for every compile is usually not worth it.
  6. Google has fancy pre-commit checks that can be run that show new lint and tidy warnings only for lines of code that has changed in the currently staged commit. This makes it easier to ask people to comply with new checks in new code without fixing all existing code. The same results appear in their fancy code review tools. Key point here is that the features feel like part of the version control and code review tools, not really the build system itself. The tools merely ask the build system for the build configuration.
johan-boule commented 2 years ago

To add some idea about point 6, we used to have a system that looked at the modified lines of code and added comments in gitlab merge requests where the change introduced new warnings, but that wasn't good enough. It was based on SonarQube's differential calculation which is broken by design: it doesn't tell you when a line of code you didn't modify gets a new warning because of changes you made elsewhere, which is a quite common scenario. So we looked for better tooling and found one done by Ericsson, called code checker, which allows you to compare the full list of warnings between the source branch and the target branch (of a merge request). Settings this up so we can do incremental build/static-analysis is not an easy task at all. First thing you need to do is to write wrapper-scripts around the compiler and clang-tidy to redirect the standard error to a file (or standard output in the case of clang-tidy). With that, after an incremental build, you still have all the warnings in the log files. Codechecker is then able to perform the diff, in a smart way where line-numbers don't matter. Gitlab can then read codechecker's diff and display it in a dedicated panel of the merge request.

boris-kolpackov commented 2 years ago

@matta @johan-boule Thanks for the thoughts.

Key point here is that the features feel like part of the version control and code review tools, not really the build system itself. The tools merely ask the build system for the build configuration.

I agree this is probably where we should start (i.e., the proper support for the Clang compilation database).

But I also wonder if this (interactive/pre-merge static analysis) is only half of it with the other half being continuous non-interactive analysis. The scenario I have in mind is a package in the repository and a new version of, say, Clang that is now capable of detecting a new bug via static analysis. Nobody is going to edit/commit the source code in this package any time soon so it would be good to have something running in the background re-analyzing the published packages and notifying the authors about new "high value" bugs, just like we do now about compiler errors/warnings.

matta commented 2 years ago

it would be good to have something running in the background re-analyzing the published packages and notifying the authors about new "high value" bugs, just like we do now about compiler errors/warnings.

Yes, post-commit analysis is very useful. Something like https://research.google/pubs/pub41342/, though Google has the a advantage of the "simplicity" of the monorepo, a constrained set of target platforms, build configurations, and toolchains. build2 could do something much simpler.