Skip to content
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

[ci] Add ids workflow for checking llvm apis have been annotated with LLVM_ABI #128370

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mcbarton
Copy link
Contributor

This PR adds a workflow which checks that llvm apis have been annotated with LLVM_ABI.

@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2025

@llvm/pr-subscribers-github-workflow

Author: None (mcbarton)

Changes

This PR adds a workflow which checks that llvm apis have been annotated with LLVM_ABI.


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

1 Files Affected:

  • (added) .github/workflows/ids-check.yml (+72)
diff --git a/.github/workflows/ids-check.yml b/.github/workflows/ids-check.yml
new file mode 100644
index 0000000000000..8ed12140d4bbc
--- /dev/null
+++ b/.github/workflows/ids-check.yml
@@ -0,0 +1,72 @@
+name: ids-check
+on:
+  pull_request:
+    branches: [main]
+  push:
+    branches: [main]
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
+  cancel-in-progress: true
+
+jobs:
+  build:
+    name: ${{ matrix.name }}
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - name: ids-check
+            os: macos-15
+
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        repository: compnerd/ids
+        path: ${{ github.workspace }}/ids
+        fetch-depth: 0
+
+    - uses: actions/checkout@v4
+      with:
+        path: ${{ github.workspace }}/llvm-project
+        fetch-depth: 0
+
+    - name: Configure and build minimal LLVM for use by ids
+      run: |
+        cmake -B ${{ github.workspace }}/llvm-project/build/ \
+              -D CMAKE_BUILD_TYPE=Release \
+              -S ${{ github.workspace }}/llvm-project/llvm/ \
+              -D LLVM_ENABLE_PROJECTS=clang \
+              -D LLVM_TARGETS_TO_BUILD="host" \
+              -G Ninja
+        cd {{ github.workspace }}/llvm-project/build/
+        ninja -t targets all | grep "CommonTableGen: phony$" | grep -v "/" | sed 's/:.*//'
+
+    - name: install llvm and lit to build ids against
+      run: |
+        brew install llvm@18
+        brew install lit@18
+
+    - name: Configure ids
+      run: |
+        cmake -B ${{ github.workspace }}/ids/build/ \
+              -S ${{ github.workspace }}/ids/ \
+               -D CMAKE_BUILD_TYPE=Release \
+               -D CMAKE_C_COMPILER=$(brew --prefix llvm@18)/bin/clang \
+               -D CMAKE_CXX_COMPILER=$(brew --prefix llvm@18)/bin/clang++ \
+               -D CMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld \
+               -D LLVM_DIR=$(brew --prefix llvm@18)lib/cmake/llvm \
+               -D Clang_DIR=$(brew --prefix llvm@18)/lib/cmake/clang \
+               -D FILECHECK_EXECUTABLE=$(brew --prefix llvm@18)/bin/FileCheck \
+               -D LIT_EXECUTABLE=$(brew --prefix lit@18)/bin/lit
+
+    - name: Build ids
+      run: |
+        cmake --build ${{ github.workspace }}/ids/build \
+              --config Release \
+              --parallel $(nproc --all)
+
+    - name: Run ids over compilation database
+      run: |
+        ${{ github.workspace }}/ids/build/bin/ids -export-macro LLVM_ABI -p ${{ github.workspace }}/llvm-project/build/compile_commands.json

Copy link

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

@mcbarton mcbarton force-pushed the ids-check-workflow branch 8 times, most recently from 0cfb07f to faec30c Compare February 23, 2025 13:44
Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long does this take to run?

@mcbarton
Copy link
Contributor Author

Hi @tstellar . The workflow I am trying to add takes around 5 minutes to run (with almost all of that being the checkout action). I created the workflow following a request by @compnerd and @vgvassilev in order to allow the work for the following issue to progress #109483 . Sorry for the large number of failed commits to try and get it to run. I'm trying to work out the exact idt command which should be run. Based on the test case added to ids here compnerd/ids#34, I feel I should have got the right command, but for some reason it doesn't work in the workflow.

@tstellar
Copy link
Collaborator

@mcbarton Can it run on the ubuntu runners? I don't think we have the capacity to run this on the macOS runners.

@mcbarton
Copy link
Contributor Author

@mcbarton Can it run on the ubuntu runners? I don't think we have the capacity to run this on the macOS runners.

@tstellar Yes, I can modify the workflow to run on the Ubuntu runners.

@andrurogerz
Copy link
Contributor

@mcbarton thanks for working on this! I took a quick look at the idt command in the job, and I see that you're not passing it the names of files to scan. In addition to passing it the comp database, I believe you also need to pass it the source files you specifically want it to scan as trailing arguments.

Something like this would scan all the files in the top commit:

idt -p ${{ github.workspace }}/llvm-project/build --export-macro=LLVM_ABI $(git diff --name-only HEAD~1 HEAD)

@mcbarton
Copy link
Contributor Author

@andrurogerz Thanks for the suggestion. My workflow doesn't currently work with the suggestion since it cannot access the HEAD~1 commit (see https://github.com/llvm/llvm-project/actions/runs/13796002477/job/38587684995?pr=128370#step:8:5) . Do you have an alternative suggestion, or can you see the mistake in my workflow as to why it cannot access the commit?

@tstellar
Copy link
Collaborator

@andrurogerz Thanks for the suggestion. My workflow doesn't currently work with the suggestion since it cannot access the HEAD~1 commit (see https://github.com/llvm/llvm-project/actions/runs/13796002477/job/38587684995?pr=128370#step:8:5) . Do you have an alternative suggestion, or can you see the mistake in my workflow as to why it cannot access the commit?

You may want to look at the https://github.com/llvm/llvm-project/blob/main/.github/workflows/pr-code-format.yml#L21 workflow. I think it might do what you want.

@mcbarton
Copy link
Contributor Author

@andrurogerz Thanks for the suggestion. My workflow doesn't currently work with the suggestion since it cannot access the HEAD~1 commit (see https://github.com/llvm/llvm-project/actions/runs/13796002477/job/38587684995?pr=128370#step:8:5) . Do you have an alternative suggestion, or can you see the mistake in my workflow as to why it cannot access the commit?

You may want to look at the https://github.com/llvm/llvm-project/blob/main/.github/workflows/pr-code-format.yml#L21 workflow. I think it might do what you want.

Thanks for the suggestion. I worked out my silly mistake after I sent that message. I wasn't in a git repo when I executed the command. The workflow is now running by based on https://github.com/llvm/llvm-project/actions/runs/13796145413/job/38588151792?pr=128370#step:8:6 . I just need to refine the diff so it only gives me the files with particular extensions.

@mcbarton mcbarton force-pushed the ids-check-workflow branch from 933016c to ea8b869 Compare March 11, 2025 20:14
@mcbarton mcbarton requested a review from tstellar March 11, 2025 20:19
Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly does this do and what are the success/fail criteria?

Do you have any idea of what the false positive and/or false negative rate looks like?

It would be good to have some documentation somewhere/leave a comment or something on failure with more information. Otherwise this is likely to just lead to warning fatigue.

@compnerd
Copy link
Member

What exactly does this do and what are the success/fail criteria?

It simply scans the specified (assumed public) headers. The declarations in a public header should be annotated properly to indicate the ABI surface of the module. idt takes a set of parameters that indicates how to determine the CPP defines appropriate for the configuration to scan for public interfaces.

Do you have any idea of what the false positive and/or false negative rate looks like?

The false positive rate should be near zero. idt is pretty conservative in general when invoked properly (it does not know how to determine a header is private or not, and that is on the developer). Additionally, it does have the ability to exclude certain symbols from being scanned if needed.

@boomanaiden154
Copy link
Contributor

The false positive rate should be near zero.

And it's clean when run over the entire monorepo already?

@mcbarton
Copy link
Contributor Author

mcbarton commented Mar 13, 2025

@boomanaiden154 I've addressed your suggestions. Can you take another look? Left it to you to set your reviews as resolved.

@mcbarton mcbarton requested a review from boomanaiden154 March 14, 2025 08:11
Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming that the underlying check is green when run across the whole repo.

@vgvassilev
Copy link
Contributor

Please don't merge this yet. There is some documentation coming soon explaining when this will be green, etc...

@compnerd
Copy link
Member

The false positive rate should be near zero.

And it's clean when run over the entire monorepo already?

No it is not, and that is the reason for putting this up early. This is to help prevent backslide while the work continues to properly annotate the codebase. This is a long process and it isn't reasonable to put up a patch to make everything clean in one go as it would touch so many headers across so many different areas.

As we make progress, we can slowly increase the scanned area so that the signal from the run is helpful.

@boomanaiden154
Copy link
Contributor

No it is not, and that is the reason for putting this up early. This is to help prevent backslide while the work continues to properly annotate the codebase. This is a long process and it isn't reasonable to put up a patch to make everything clean in one go as it would touch so many headers across so many different areas.

So if someone puts up a patch that touches a file that has not been annotated yet (completely unrelated to the annotation effort), this check would flag it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants