[workflows] Add post-commit job that periodically runs the clang static analyzer

[tstellar]

This job will run once per day on the main branch, and for every commit on a release branch. It currently only builds llvm, but could add more sub-projects in the future.

OpenSSF Best Practices recommends running a static analyzer on software before it is released: https://www.bestpractices.dev/en/criteria/0#0.static_analysis

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

This job will run once per day on the main branch, and for every commit on a release branch. It currently only builds llvm, but could add more sub-projects in the future.

OpenSSF Best Practices recommends running a static analyzer on software before it is released: https://www.bestpractices.dev/en/criteria/0#0.static_analysis

---

Full diff: https://github.com/llvm/llvm-project/pull/94106.diff

1 Files Affected:

• (added) .github/workflows/ci-post-commit-analyzer.yml (+64)

diff --git a/.github/workflows/ci-post-commit-analyzer.yml b/.github/workflows/ci-post-commit-analyzer.yml
new file mode 100644
index 0000000000000..b7ee832b8e8ea
--- /dev/null
+++ b/.github/workflows/ci-post-commit-analyzer.yml
@@ -0,0 +1,64 @@
+name: Post-Commit Static Analyzer
+
+permissions:
+  contents: read
+
+on:
+  push:
+    branches:
+      - 'release/**'
+    paths:
+      - 'llvm/**'
+  pull_request:
+    paths:
+      - '.github/workflows/ci-post-commit-analyzer.yml'
+  schedule:
+    - cron: '30 0 * * *'
+
+concurrency:
+  group: >-
+    llvm-project-${{ github.workflow }}-${{ github.event_name == 'pull_request' &&
+      ( github.event.pull_request.number || github.ref) }}
+  cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }}
+
+jobs:
+  post-commit-analyzer:
+    if: >-
+      github.repository_owner == 'llvm' &&
+      github.event.action != 'closed'
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Checkout Source
+        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
+
+      - name: Install Dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install \
+            cmake \
+            ninja-build \
+            perl \
+            clang-tools \
+            clang
+
+      - name: Configure
+        run: |
+          scan-build \
+              --use-c++=clang++ \
+              --use-cc=clang \
+              cmake -B build -S llvm -G Ninja \
+                  -DLLVM_ENABLE_ASSERTIONS=ON \
+                  -DLLVM_BUILD_LLVM_DYLIB=ON \
+                  -DLLVM_LINK_LLVM_DYLIB=ON \
+                  -DCMAKE_BUILD_TYPE=Release
+
+      - name: Build
+        run: |
+           scan-build -o analyzer-results --use-c++=clang++ --use-cc=clang ninja -v -C build
+
+      - name: Upload Results
+        uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 #v4.3.0
+        with:
+          name: analyzer-results
+          path: 'analyzer-results/**/*'
+

[boomanaiden154]

Thanks for doing this.

I guess we'll see from the PR run how long this takes and what the results are like. From what I've heard, the clang static analyzer produces false positives that can be hard to fix. I'm not sure how that anecdote generalizes to the LLVM code base though.

Also, is there a reason for using clang static analyzer over something more basic like clang tidy? I think they both would fit the OpenSSF definition of static analysis (although haven't looked into it). I guess CSA does do more in depth analysis and will probably find more things.

Even if it's noisy, just having it run once a day post commit so that people who are interested can look at the results seems like a decent idea.

[tstellar]

Thanks for doing this.

I guess we'll see from the PR run how long this takes and what the results are like. From what I've heard, the clang static analyzer produces false positives that can be hard to fix. I'm not sure how that anecdote generalizes to the LLVM code base though.

Also, is there a reason for using clang static analyzer over something more basic like clang tidy? I think they both would fit the OpenSSF definition of static analysis (although haven't looked into it). I guess CSA does do more in depth analysis and will probably find more things.

Even if it's noisy, just having it run once a day post commit so that people who are interested can look at the results seems like a decent idea.

This is really only for informational purposes at this point. I was going to watch it for a while to see which of the checks are helpful and which ones are prone to false positives. I'm open to changing the set of checks and also using clang-tidy if someone has a good recommendation for which checks to run.

[steakhal]

Even if it's noisy, just having it run once a day post commit so that people who are interested can look at the results seems like a decent idea.

This is really only for informational purposes at this point. I was going to watch it for a while to see which of the checks are helpful and which ones are prone to false positives. I'm open to changing the set of checks and also using clang-tidy if someone has a good recommendation for which checks to run.

Usually, clang-tidy is good for context insensitive rules, basically for the cases that can be easily detected by looking at the AST.
CSA is usually better if some state-machine describes a bug condition, which is attached to some value. For example, if we have a pointer to some resource, use it and then close/release it. And this is tracked across different statements and conditions.

W.r.t the FP TP rate of the checks, the default enabled checkers is usually a fairly conservative selection, which should produce low FP rate in practice. There are useful checkers and combinations that are not enabled by default, but considered to be fairly mature otherwise. At Sonar, we use CSA as our symbolic execution engine backend for our rules where it's relevant, I'll ask if we can publish what subset of checkers we use in production. (SonarCloud is free for open-source projects, example).
I can also see the benefit of having something from directly in-house, like scan-build.

Besides the scan-build, there is also CodeChecker which serves as a frontend for static analyzer invocations. They already maintain checker "profiles" that categorizes the checkers. /cc @NagyDonat representing them. He could probably propose some refined list for us about what checkers work for them in practice.

For now, I think even the most basic setup is better than having nothing. So I'm all for having this PR.

---

FYI I also considered contributing something like this, but I found no policies around introducing CI workflows, and I'm aware that even if this is an open-source and well-known project, at the end of the day we probably only have limited CI time and/or some companies pay for that time - like build bots.

BTW don't we already have some nightly for scan-build? https://llvm.org/reports/scan-build/
Do you plan to replace that?

[tstellar]

@steakhal Thanks for the information. CodeChecker looks interesting, but setting up a server is a little too much work for me.

FYI I also considered contributing something like this, but I found no policies around introducing CI workflows, and I'm aware that even if this is an open-source and well-known project, at the end of the day we probably only have limited CI time and/or some companies pay for that time - like build bots.

We don't have any official policies about adding new CI. GitHub let's us use up-to 60 concurrent action runners, which it provides for free. I think if the job is small or it doesn't need to run for every PR then there is probably capacity for t.

BTW don't we already have some nightly for scan-build? https://llvm.org/reports/scan-build/ Do you plan to replace that?

I think that is generated as part of apt.llvm.org, but I don't plan on replacing it.

[haoNoQ]

I'm very excited to see this!

A few years ago we've actually had some work done to improve quality on LLVM specifically, and even built an LLVM-specific checker (it finds dyn_casts that aren't checked for null before use): https://discourse.llvm.org/t/gsoc-2019-apply-the-clang-static-analyzer-to-llvm-based-projects-final-report/52919/4

cc @Xazax-hun @RKSimon @sylvestre @csaba-dabis

IIRC it was Sylvestre who was maintaining the old scan-build output bot, and Simon has been voluntarily fixing these warnings for a while.

[RKSimon]

We used to test on coverity as well: https://scan.coverity.com/projects/llvm - but that broke some time ago and has proven tricky to keep running

[haoNoQ]

Not sure but I suspect that you should remove the /** part now. Because now that you removed scan-build nobody is creating that subdirectory with today's date anymore. All the HTML files go straight into analyzer-results now.

[haoNoQ]

I don't see any other potential problems, everything makes sense to me and I'm very excited!

[haoNoQ]

Ok looks like something crashed? The log isn't very informative, could also be out-of-memory or something of that nature. can we pass some flags to make it generate reproducers?

Actually scan-build does this out of the box by re-running the crashed invocation and put the crash dump and the repros into the results folder, in a sub-folder named failures. We could do the same in our launcher script, and then scan-build --generate-index-only might even pick them up and show them on the index.html page!

It may also be a good idea to || true the static analyzer invocation, so that static analyzer crashes don't block the rest of the build. Static analyzer invocations don't depend on each other, their output doesn't need to "link" to each other, so it's ok to ignore these crashes. Also it's not like we're using a freshly built clang we can fix to unblock ourselves.

Side note, if we used the clang-tidy workflow with compilation databases, it'd naturally allow us to use freshly built clang for analysis, because all analysis would be run strictly after the normal build is finished. (We could use that workflow even without clang-tidy, just grep all run-lines from the compilation database and add --analyze etc. to them.)

[tstellar]

Ok looks like something crashed? The log isn't very informative, could also be out-of-memory or something of that nature. can we pass some flags to make it generate reproducers?

I think it's out of memory, I noticed this line:

2024-06-06T02:14:25.6030984Z Killed

[haoNoQ]

Hmm, in this case could this also be the source of performance problems? Like it's swapping or something? Should we try to decrease our -j?

If it's just one specific file that goes OOM every single time, then we can exclude that file from analysis by wrapping it in:

#ifndef __clang_analyzer__
// the file
#endif

But in this case I should take a look at this file. It could be that there's some really weird code that dodges all our limits. (Historically it's been very long initializer lists that we try to represent perfectly in symbolic memory and they count as 1 node for the purposes of max-nodes.)

And yeah we should || true the analyzer command either way. No reason to block on this.

[haoNoQ]

The static analyzer is several times more memory-intensive than the compiler, just like it's several times slower than the compiler. But the size of the file is usually not an issue: we deallocate all our analysis-related data structures after every top-level function, going back down to the AST. So if one specific file consumes unusually large amount of memory, it's not because the file is big; it's because one specific function in this file triggers a bug. You can also display all top-level functions with -Xclang -analyzer-display-progress. The last one printed is the one in which we got stuck. But this could be too verbose for an everyday buildbot job.

Let me take a look if I can make reproducers work.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[tstellar]

@haoNoQ I think RISCVFoldMasks.cpp has a pathological case that uses a lot of memory.

I've made a few changes based on your suggestions. I split build and analyze into completely separate steps now. This should allow us to avoid building some unnecessary targets and the script I use to run the analyzer will keep running if one of the jobs fail.

[haoNoQ]

Aha yeah either way LGTM!

I'll take a look at that crash in a moment.

At some point we should just merge your python script directly into scan-build so that you didn't need to reinvent that wheel.

[boomanaiden154]

LGTM.

It seems like we aren't utilizing all the time now (~180/360 minutes). I think that should probably be fine. We can always adjust limits later as we have a better feel for the job duration.