Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions .github/workflows/ids-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
name: ids-check
on:
pull_request:
types:
- opened
- synchronize
- reopened
- closed
push:
branches:
- 'main'
- 'release/**'

permissions:
contents: read

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
cancel-in-progress: true

jobs:
build:
if: github.repository_owner == 'llvm'
name: ids-check
runs-on: ubuntu-24.04
Copy link
Member

Choose a reason for hiding this comment

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

We really should be building this on Windows. While ids will properly scan on Linux, it does require that you have configured the build for the host that you want to scan for - that is, we want to be building a windows binary. I do not know if the EULA for MSVC allows you to have the installation on Linux for that cross-compilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to run on Windows or compile for a Windows host? The symbol visibility functionality also applies to Linux. I'd expect that running this on Linux is simpler and faster. (Running for any one target will miss conditionally compiled code, but that's true regardless of which one you pick.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Running on Linux should be faster.

We're also capacity constrained on Windows, so running it there (especially on every PR) will put additional pressure there.

Copy link
Member

Choose a reason for hiding this comment

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

The scanning occurs through the clang tooling, which will build as if it were building for Linux on Linux. To build for Windows on Linux, you need to ensure that you are setting up the cross-compilation appropriately: i.e. you configure and have available the Windows SDK, Microsoft C runtime, etc. If we can arrange for that, doing this build on Linux would be fine.

Copy link
Contributor

@nikic nikic Aug 14, 2025

Choose a reason for hiding this comment

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

You still haven't explained why it is important to target Windows. What's the problem with building on Linux for Linux (apart from conditionally compiled code, which will always be missed in one direction or the other)?

Copy link
Contributor Author

@mcbarton mcbarton Aug 14, 2025

Choose a reason for hiding this comment

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

This is not a comment either way with regards to which runner to use. I will implement the ids/idt check on whatever platform is chosen. This is just an update as to where the ids workflow stands for both, before I go on leave for the next 2 weeks. I have managed to get ids/idt to build on Linux. It takes around 2-3 minutes to build ids (see https://github.com/llvm/llvm-project/actions/runs/16962718956/job/48079138233). I tested to see whether the Linux based workflow would detect if a PR had a missing LLVM_ABI by removing one from a class, and it didn't detect it, so something is wrong with the flags currently chosen.

Despite trying all day I am yet to get a successful Windows build of ids on either the Windows x86 or arm runners. If I do manage to get it to work, then it will likely take 5-10 minutes for the build and check based on my progress so far (there seems to be great variance in the speed of the windows runners even of the same architecture).

Copy link
Member

Choose a reason for hiding this comment

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

You still haven't explained why it is important to target Windows. What's the problem with building on Linux for Linux (apart from conditionally compiled code, which will always be missed in one direction or the other)?

The macro expansions are platform specific: Linux doesn't use __declspec(dllexport) and __declspec(dllimport) which have different semantics from __attribute__((__visibility__(...))). We would get close with just a Linux build, but it won't be exact. ids basically just parses the source code as if it were the target you specify and then attempts to identify the attributes on the interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide an example where ids will produce different output when targeting Linux and Windows?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that I have an example off hand, I remember that we did find a few different cases of the handling. But, I suppose that some coverage is better than none.

DLL Storage and visibility are subtly different and while building on Linux may be easier, I expect that we will need to do more reverts for the Linux based pre-merge tests than with the Windows based pre-merge tests.


steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
repository: compnerd/ids
path: ${{ github.workspace }}/ids
fetch-depth: 0

- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
path: ${{ github.workspace }}/llvm-project
fetch-depth: 2

- name: install dependencies
run: |
sudo apt install -y clang-19 ninja-build libclang-19-dev
pip install lit
- 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" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-G Ninja
cd ${{ github.workspace }}/llvm-project/build/
ninja -t targets all | grep "CommonTableGen: phony$" | grep -v "/" | sed 's/:.*//'
- name: Configure ids
run: |
cmake -B ${{ github.workspace }}/ids/build/ \
-S ${{ github.workspace }}/ids/ \
-D CMAKE_BUILD_TYPE=Release \
-D CMAKE_C_COMPILER=clang \
-D CMAKE_CXX_COMPILER=clang++ \
-D CMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld \
-D LLVM_DIR=/usr/lib/llvm-19/lib/cmake/llvm/ \
-D Clang_DIR=/usr/lib/llvm-19/lib/cmake/clang/ \
-D FILECHECK_EXECUTABLE=$(which FileCheck-19) \
-D LIT_EXECUTABLE=$(which lit)
- name: Build ids
run: |
cmake --build ${{ github.workspace }}/ids/build \
--config Release \
--parallel $(nproc --all)
- name: Run ids over compilation database
run: |
cd ${{ github.workspace }}/llvm-project
export H_OR_CPP_FILES_CHANGED_LAST_COMMIT=$(git diff --name-only HEAD~1 HEAD -- 'llvm/include/llvm/**/*.h' ':!llvm/include/llvm/Debuginfod/' ':!llvm/include/llvm/Demangle/' )
if [ ! -z "${H_OR_CPP_FILES_CHANGED_LAST_COMMIT}" ]; then
for file in $H_OR_CPP_FILES_CHANGED_LAST_COMMIT; do
${{ github.workspace }}/ids/build/bin/idt -p ${{ github.workspace }}/llvm-project/build/compile_commands.json --apply-fixits --inplace --export-macro=LLVM_ABI --include-header="llvm/include/llvm/Support/Compiler.h" --extra-arg="-DLLVM_ABI=__attribute__((visibility(\"default\")))" --extra-arg="-Wno-macro-redefined" $file
done
fi
export H_OR_CPP_FILES_CHANGED_LAST_COMMIT=$(git diff --name-only HEAD~1 HEAD -- 'llvm/include/llvm-c/**/*.h' )
if [ ! -z "${H_OR_CPP_FILES_CHANGED_LAST_COMMIT}" ]; then
for file in $H_OR_CPP_FILES_CHANGED_LAST_COMMIT; do
${{ github.workspace }}/ids/build/bin/idt -p ${{ github.workspace }}/llvm-project/build/compile_commands.json --apply-fixits --inplace --export-macro=LLVM_C_ABI --include-header="llvm/include/llvm-c/Visibility.h" --extra-arg="-DLLVM_C_ABI=__attribute__((visibility(\"default\")))" --extra-arg="-Wno-macro-redefined" $file
done
fi
export H_OR_CPP_FILES_CHANGED_LAST_COMMIT=$(git diff --name-only HEAD~1 HEAD -- 'llvm/include/llvm/Demangle/**/*.h' )
if [ ! -z "${H_OR_CPP_FILES_CHANGED_LAST_COMMIT}" ]; then
for file in $H_OR_CPP_FILES_CHANGED_LAST_COMMIT; do
${{ github.workspace }}/ids/build/bin/idt -p ${{ github.workspace }}/llvm-project/build/compile_commands.json --apply-fixits --inplace --export-macro=DEMANGLE_ABI --include-header="llvm/Demangle/Visibility.h" --extra-arg="-DEMANGLE_ABI=__attribute__((visibility(\"default\")))" --extra-arg="-Wno-macro-redefined" $file
done
fi
git diff --quiet
if [ $? -ne 0 ]; then
echo "Apply the following diff to fix the LLVM_ABI annotations"
git diff
exit 1
fi
2 changes: 1 addition & 1 deletion llvm/include/llvm/ADT/Any.h
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@

namespace llvm {

class LLVM_ABI Any {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change will be reverted before the PR goes in. It has only been made for now as the workflow should fail when changes like this are made.

class Any {

// The `Typeid<T>::Id` static data member below is a globally unique
// identifier for the type `T`. It is explicitly marked with default