-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[workflows] Add post-commit job that periodically runs the clang static analyzer #94106
Conversation
OpenSSF Best Practices recoomends running a static analyzer on software before it is released: https://www.bestpractices.dev/en/criteria/0#0.static_analysis
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-github-workflow Author: Tom Stellard (tstellar) ChangesThis 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:
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/**/*'
+
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Usually, clang-tidy is good for context insensitive rules, basically for the cases that can be easily detected by looking at the AST. 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). 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/ |
@steakhal Thanks for the information. CodeChecker looks interesting, but setting up a server is a little too much work for me.
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.
I think that is generated as part of apt.llvm.org, but I don't plan on replacing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_cast
s 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.
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 |
Added ccache support and used our pre-built clang to help speed up the build. Also passing -analyzer-config max-nodes=75000 to scan-build now.
709feeb
to
3019e70
Compare
if: always() | ||
with: | ||
name: analyzer-results | ||
path: 'build/analyzer-results/**/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any other potential problems, everything makes sense to me and I'm very excited!
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 It may also be a good idea to 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 |
I think it's out of memory, I noticed this line:
|
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 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:
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 And yeah we should |
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 Let me take a look if I can make reproducers work. |
Also continue analyzing if one job fails.
✅ With the latest revision this PR passed the Python code formatter. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5e8fae8
to
c5bdc29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…ic analyzer (llvm#94106) 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 Signed-off-by: Hafidz Muzakky <[email protected]>
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