> But why not for instance use a build system in some "container"?

I am not sure how this helps.

> I think the project could "bother" contributors with something like that, couldn't it?

Which project?

> An embedded C developer I've talked with quite often on some other forum, who imho is quite competent, said that Coverity is a poor tool that generates way too much false negatives and overlooks at the same time glaring issues.

He likely violated a license agreement with Coverity, since no one is allowed to say anything comparing Coverity to anything else.

> Said that's mostly an issue with all OpenSource tools for static C analysis.

I have been filing bug reports.

> OTOH the commercial ones are very expensive usually, with a target market of critical things like aviation of safety systems in cars and military use, places where they spend billions on projects. Nothing there for the average company, and especially not for (frankly often underfunded) OpenSource projects.

So you understand my pain.

> CodeQL? It's mostly an semantic search and replace tool, as I know? Is it that helpful? (I had a look, but the projects I'm working on don't require it. One would just use the IDE. No need for super large-scale refactorings, across projects, in our case).

I have never heard about this function. It is a static analyzer whose checks are written in the CodeQL language. However, it is very immature. When github acquired it, they banished the less reliable checks to the extended-and-security suite, leaving it only with about ~50 checks for C/C++ code. Those catch very little, although in the rare instances that they do catch things, the catches are somewhat amazing. Unfortunately, at least one of those checks provides technically correct, yet difficult to understand, explanations of the problem, so most developers would dismiss its reports as false positives despite it being correct:

https://github.com/github/codeql/issues/11744

There are probably more issues like that, but I have yet to see and report them.

> SonarCloud, hmm… This one I've used (around web development though). But am not a fan of. It bundles other "scanner" tools, with varying quality and utility. At least what they had for the languages I've actively used it was mostly about "style issues". And when it showed real errors, the IDE would do the same… (The question then is how this could be committed in the first place. But OK, some people just don't care. For them you need additional checks like SonarCloud I guess.)

It is supposed to be able to integrate into github's code scanning feature, so any newly detected issues are reported in the PR that generated them. Anyway, it is something that I am considering. I wanted to use it much sooner, but it required authorization to make changes to github on my behalf, which made me cautious about the manner in which I try it. It is basically at the bottom of my todo list right now.

> Wouldn't it be easy to add at least this to the build by using some "build container"?

I do not understand your question. To use it, we need a few things:

1. To be able to show any newly introduced defect reports in the PR that generated them shortly after it was filed.

2. To be able to scan the kernel modules since right now, it cannot due to a bad interaction between the build system and how compiler interposition is done. As of a few days ago, I have a bunch of hacks locally that enable kernel module scans, but this needs more work.

3. An easy way to mark false positives without doing commits.

Without those things in place, it is a dead on arrival proposition. OpenZFS already tried using CPPCheck without #1 and #3 in place and it was so painful that the project reversed course. That happened while I was on a multi-year sabbatical from the project, so I only know about it from seeing traces of it in the repository and asking others who were around for it about what happened.

> Well, that's why I think something equivalent to `-Wall -Werror` should be switched on before writing the first line of code, in any language.

OpenZFS has had that in place for more than a decade. I do not know precisely when it was first used (although I could look if anyone is particularly interested), but my guess is 2008 when ZFSOnLinux started. Perhaps it was done at Sun before then, but both events predate me. I became involved in 2012 and it is amazing to think that I am now considered one of the early OpenZFS contributors.

Interestingly, the earliest commits in the OpenZFS repository referencing static analysis are from 2009 (with the oldest commit being from 2008 when ZFSOnLinux started). Those commits are ports of changes from OpenSolaris based on defect reports made by Coverity. There would be no more commits mentioning static analysis until 2014 when I wrote patches fixing things reported by Clang's static analyzer. Coverity was (re)introduced in 2016.

As far as the current OpenZFS repository is concerned, knowledge of static analysis died with OpenSolaris and we lost an entire form of QA until we rediscovered it during attempts to improve QA years later.

That said, if you have suggestions on QA, I am willing to listen to them, although keep in mind that it takes time for me to do things, even if I think they are good ideas, and I already have a large number of ideas to implement since returning to the project earlier this year.

> But I guess I will stay with engraving my data into solid rock. Proven for at least hundred thousand years.

That method is no longer reliable due to acid rain. You would need to bury it in a tomb to protect it from acid rain. That has the pesky problem of the pointers being lost over time.

> At least someone needs to preserve the cat pictures and meme of our current human era for the cockroach people of the distant future. I'm not sure they will have a compatible Linux kernel and compiler available to build the ZFS drivers, or even punch card readers…

Github's code vault found a solution for that, although they used it on source code:

https://github.com/github/archive-program/blob/master/GUIDE....

I vaguely recall another effort trying to include the needed hardware in time capsules, but I could be misremembering.

> > But why not for instance use a build system in some "container"?

> I am not sure how this helps.

> > I think the project could "bother" contributors with something like that, couldn't it?

> Which project?

OpenZFS. But this is kind of a misunderstanding on my side. I thought you were talking about convince for local developers. But you're talking about a CI system? Did I now understand correctly?

But still a local developer needs a local build environment that is similar to the one on the CI which will do the checks later on. It's a common practice to bundle the whole tool-chain and all kind of checkers and scanners in a "build container". You can run it than locally as on the CI.

But how to do bare metal tests on GitHub I actually don't know. Kernel divers are something special I guess. Not the usual workload that can be completely put into a container. One needs at least some VM setup. Never did that on GitHub.

> > An embedded C developer I've talked with quite often on some other forum, who imho is quite competent, said that Coverity is a poor tool that generates way too much false negatives and overlooks at the same time glaring issues.

> He likely violated a license agreement with Coverity, since no one is allowed to say anything comparing Coverity to anything else.

LOL

> I have never heard about this function. It is a static analyzer whose checks are written in the CodeQL language.

Need to take a second look I guess. I had it differently in mind, but OK.

Thanks for the pointer.

> That said, if you have suggestions on QA, I am willing to listen to them, although keep in mind that it takes time for me to do things, even if I think they are good ideas, and I already have a large number of ideas to implement since returning to the project earlier this year.

I'm not sure I can say much without having actually deeper insides into issues with the current QA setup.

It sounded like it would be difficult to introduce tools for static analysis. But I'm not completely sure whether this is more a technical issue or a developer convenience and workflow thingy.

At least some of such tools can be integrated into a fully packaged build tool-chain, like I said. This way you can rule out for example that someone commits stuff that triggers false positives in some code analysis tool that you can run also against your local build. Of course you still need checks on the CI in place. But those checks should actually never trigger by something that someone should have seen and resolved already locally (and if it does trigger this needs investigation usually anyway as the local build does not mirror the CI build in every aspect any more). This would relief the need to "ignore" checks on the CI without commits, I guess. Just do the checks you can do locally locally. Deliver the tools to do so in a convenient "package" ("container", VM image / config, whatever; you know, something like Vagrant).

Showing test results in PRs or even outright reject PRs based on test results can be done in GitHub with Actions. One can also trigger Actions by special comments, useful for example for checks and benchmarks that only need to run occasionally when something related changes. But I guess you know all that… :-)

Regarding code analysis: I think the stuff that Microsoft Research does is also interesting, and wasn't mentioned yet. But I'm not sure how practically relevant this is (especially in Unix land). They have at least on paper some tools for C code analysis. But no clue how stuff works in reality. Never heard directly from anybody who used this things.

> OpenZFS. But this is kind of a misunderstanding on my side. I thought you were talking about convince for local developers. But you're talking about a CI system? Did I now understand correctly?

I was actually asked by another OpenZFS developer about a good local tool for this. I did not have an answer then and I still do not have a great answer for it now, but I am getting closer to one. Some build system patches + Clang's static analyzer + codechecker seems to be the most promising combination since that would not only be able to cross translation unit analysis, but also be able to do differential reporting so only new reports are visible, but I still need to figure out all of the details.

Also, I suspect the end result will be far less useful in practice than it sounds because of just how long the analysis takes. We would need a way to do incremental analysis like we can do incremental builds for it to be truly useful, and I currently do not have an answer for incremental analysis, although I suspect maybe something could be done in CodeChecker based on filesystem time stamps.

Supposedly PVS Studio can do that, but as I explained my previous comment (in an edit that I probably made after you initially read it), PVS Studio is unlikely to be an option beyond its free trial period.

That said, QA would be better achieved by integrating these tools into our continuous integration infrastructure so that they could be run whenever a pull request is done so that all newly reported issues are made available to all reviewers and we can avoid merging things that introduce regressions that static analyzers can catch in the first place.

> But still a local developer needs a local build environment that is similar to the one on the CI which will do the checks later on. It's a common practice to bundle the whole tool-chain and all kind of checkers and scanners in a "build container". You can run it than locally as on the CI.

It is easy to do builds on any Linux / FreeBSD machine. The test suite really should be run in a VM since running experimental code on your kernel is not advisable and is not always feasible. Machines that have root on ZFS cannot have that code loaded into the running kernel without significant inconveniences, which include breaking the system if you mess up in a way that completely breaks the driver. You can still use the ZFS stochastic testing tool called ztest. That does testing on a userland version of ZFS, although it is different from running the test suite.

> But how to do bare metal tests on GitHub I actually don't know. Kernel divers are something special I guess. Not the usual workload that can be completely put into a container. One needs at least some VM setup. Never did that on GitHub.

I think we are talking about different things here. github can integrate with buildbots and has its own runners that you can use via github workflows, but running the test suite has nothing to do with static analyzers, which I thought were the focus of the discussion.

> Need to take a second look I guess. I had it differently in mind, but OK.

I revised my response to ask if you had mistaken CodeQL for Coccinelle.

> It sounded like it would be difficult to introduce tools for static analysis.

It is infrastructure work. It is doable, but it takes time. I have been working on it. The addition of CodeQL to github pull requests was the first thing to come out of that.

> But I'm not completely sure whether this is more a technical issue or a developer convenience and workflow thingy.

If the developer is inundated with reports, it is useless. That is why we need to ensure that developers only see new reports from their code changes. We also need to be able to mark existing reports as false positives so that we do not have a bunch of unresolved reports.

> At least some of such tools can be integrated into a fully packaged build tool-chain, like I said. This way you can rule out for example that someone commits stuff that triggers false positives in some code analysis tool that you can run also against your local build.

Commits can only be done by a select few people and they are only done following code review. Asking the reviewers to download the code locally to redundantly run all of these tools on their local machines (which also implies inundating them with reports from pre-existing issues) is a fantastic way to cause developer burn out. That might sound silly, but real problems occur when you require developers to do menial tasks that require them to process huge amounts of mostly useless information.

I have been through all of that myself and the burnout problem hit me. Excessive local testing is also a productivity killer, which becomes a morale killer when you put your tested patch in a pull request and then the buildbot not only finds a problem with it, but then demonstrates that all of your local testing was a waste of time since you now need to start from the beginning in retesting everything.

It is far better to have the tools' reports added to the pull request by a machine for the reviewers to see in the PR, with pre-existing issues filtered from those reports. It avoids the "please end my suffering now" part of the development process that occurs when developers manually run all of this locally every single time they make a small change.

> Of course you still need checks on the CI in place. But those checks should actually never trigger by something that someone should have seen and resolved already locally (and if it does trigger this needs investigation usually anyway as the local build does not mirror the CI build in every aspect any more).

They do all the time. You simply cannot test all of the architectures, OS version combinations, etcetera locally that are tested by the continuous integration infrastructure.

> This would relief the need to "ignore" checks on the CI without commits, I guess.

I do not understand.

> Just do the checks you can do locally locally.

I used to do this. My productivity was several times lower, I still missed things that the continuous integration software caught and I burned myself out doing menial work by needing to manually run all of this. I really do not recommend it.

Now I do much less of this and rely on the buildbot to do the menial work.

> Deliver the tools to do so in a convenient "package" ("container", VM image / config, whatever; you know, something like Vagrant).

I have a suspicion nobody would use it since everything they need is already fairly simple to setup.

> Showing test results in PRs or even outright reject PRs based on test results can be done in GitHub with Actions.

OpenZFS already does this.

> One can also trigger Actions by special comments, useful for example for checks and benchmarks that only need to run occasionally when something related changes. But I guess you know all that…

I did not know that part, but I find it to be more useful to always run all tests all the time.

> Regarding code analysis: I think the stuff that Microsoft Research does is also interesting, and wasn't mentioned yet. But I'm not sure how practically relevant this is (especially in Unix land). They have at least on paper some tools for C code analysis. But no clue how stuff works in reality. Never heard directly from anybody who used this things.

If they published them for third party use, I would be willing to look.

> Some build system patches + Clang's static analyzer + codechecker seems to be the most promising combination since that would not only be able to cross translation unit analysis, but also be able to do differential reporting so only new reports are visible, but I still need to figure out all of the details. Also, I suspect the end result will be far less useful in practice than it sounds because of just how long the analysis takes. We would need a way to do incremental analysis like we can do incremental builds for it to be truly useful, and I currently do not have an answer for incremental analysis, although I suspect maybe something could be done in CodeChecker based on filesystem time stamps.

I use CodeChecker with ClangSA, CTU enabled and with Z3 for refutation. The code base is smaller so incremental checking is less a concern to me.

Still, we had a look. CC allows taking a list of files, and updating those files analysis only while keeping the existing results for other non listed files.

But unless something has changed this partial analysis just do what it's told, analyzing only the files given on the CLI. With CTU this may miss side effects: a modified file may impact other files using its function for example. It's possible to use CC own CTU info to derive these dependencies and extend the list of files.

Then there are modified header files, with the usual inclusion dependencies.

So if it's not provided "out of the box", it should be possible to have a layer on top taking a list of changed files, extending it with both CTU and header dependencies, and passing the extended list to CC for a safe update.

> since no one is allowed to say anything comparing Coverity to anything else

A product where the company is afraid of comparative reviews implies it just isn't a competitive product.

Feel free to compare D with any other programming language.

> An embedded C developer I've talked with quite often on some other forum, > who imho is quite competent, said that Coverity is a poor tool that > generates way too much false negatives and overlooks at the same time > glaring issues. He was not happy about it. Said that's mostly an issue > with all OpenSource tools for static C analysis. OTOH the commercial ones > are very expensive usually, with a target market of critical things like > aviation of safety systems in cars and military use, places where they > spend billions on projects. Nothing there for the average company, and > especially not for (frankly often underfunded) OpenSource projects.

I don't have any experience with Coverity, but used another commercial static analyzer (SA) that I won't name in case there are similar legal limitations as with Coverity ;)

With the commercial tool I have the same problem you mention: too many false alarms. Most reports are a waste of time really. In the end, people tend to ignore the tool.

The best results I get with Clang SA, used through CodeChecker. Very few false alarms, and usually at least code smells. It's not sound of course, but is free and could spot a few nasty bugs. I recommend trying CC+ClangSA.

One important point is the ability to do cross files analysis. It is very common to use a value from a function in another file. With a "one file at a time" analysis no assumption can be made on this value range, so the analyzer must be conservative. This leads to a lot of false alarms in practice.

With cross file analysis (called "cross translation units" or CTU analysis in Clang SA), the SA can propagate constraints along paths traversing multiple files. This makes a huge difference in my experience. As I explained in another comment, this is well suited for an application: all paths come from `main`. For a library the result will depend on the test application(s) provided to the SA: the call to the library under test must cover all the function domains for the analysis not to miss some issue. Otherwise, it may use too restrictive constraints from the UT that are not relevant to real life use.

But it may not be enough: the commercial tool I use do make cross file analysis. It seems more limited than Clang SA though, and misses a lot.

Another thing that help IMHO is the use of Z3 as a checker. ClangSA uses a simple and fast range based analysis. As far as SA goes, this is a bit primitive and not as powerful as polyhedral analysis for example (which is more expensive).

But when Clang SA range analysis finds an issue, it is cross checked with the Z3 SMT solver, which can deal with more complex constraints, and possible rejected there. I do embedded development, and Z3 can understand bit fields and bit operations for example (trivial for a SMT solver with bit vectors). I never tried with and without using Z3 in this way, but I assume it helps.

In any case, I'm very happy with the results I get from ClangSA+CC with CTU and Z3 enabled (check your LLVM toolchain for Z3 support, OK in Debian stable for example). It's much better in practice than the costly commercial tool we still use, for now.

For cross compilation, there may be a few Clang/GCC differences to work-around. CodeChecker already handle some, but maybe not all. It's been manageable in our case. For Linux based development CC should work fine out of the box.