Add a clang-tidy CI job for Windows-only code and fix issues

.github/scripts/guarded_sources.py

@@ -0,0 +1,55 @@
+#!/usr/bin/env python3
+"""List sources whose preprocessor conditionals reference the given macros.
+
+Usage: guarded_sources.py <macro-alternation> [--headers]
+
+Prints path regexes for every .cpp file (or, with --headers, a single
+alternation of every .h file) under framework/, tools/ and layer/ that
+contains a preprocessor conditional naming one of the macros, e.g.:
+
+    guarded_sources.py 'WIN32|_WIN32|D3D12_SUPPORT'
+
+The output is meant for run-clang-tidy.py: the .cpp regexes as its source
+filter arguments and the header alternation as its -header-filter value.
+CI uses this to run clang-tidy on Windows over just the code that the
+Linux clang-tidy job never sees.
+"""
+
+import re
+import subprocess
+import sys
+
+
+def main():
+    if len(sys.argv) < 2:
+        sys.exit(__doc__.splitlines()[2])
+    # Keep '\n' as-is on Windows: a '\r\n' separator would leave a stray
+    # '\r' in the regexes after shell word splitting.
+    sys.stdout.reconfigure(newline='\n')
+    macros = sys.argv[1]
+    pattern = rf'#\s*(if|ifdef|ifndef|elif).*\b({macros})\b'
+    result = subprocess.run(
+        ['git', 'grep', '-lE', pattern, '--',
+         'framework', 'tools', 'layer', ':!framework/generated'],
+        capture_output=True,
+        text=True)
+    # git grep exits 1 on "no matches", which is not an error here.
+    if result.returncode > 1:
+        sys.stderr.write(result.stderr)
+        sys.exit(result.returncode)
+    files = result.stdout.split()
+
+    def path_regex(path):
+        # Match both separators so the regexes also work against the paths
+        # in a Windows compile database.
+        return re.escape(path).replace('/', r'[/\\]')
+
+    if '--headers' in sys.argv[2:]:
+        headers = [path_regex(f) for f in files if f.endswith('.h')]
+        print('|'.join(headers) or '^$')
+    else:
+        print('\n'.join(path_regex(f) for f in files if f.endswith('.cpp')))
+
+
+if __name__ == '__main__':
+    main()

.github/workflows/ci_build.yml

@@ -62,6 +62,66 @@ jobs:
           -warnings-as-errors='*' \
           '(framework/(?!generated/)|tools/|layer/)'
 
+  clang-tidy-windows:
+    name: "Check Windows code quality with clang-tidy"
+    runs-on: windows-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        submodules: 'recursive'
+    - name: Set up MSVC environment
+      uses: ilammy/msvc-dev-cmd@v1
+    - name: Get run-clang-tidy
+      shell: bash
+      run: |
+        # Use the runner's LLVM clang-tidy, which matches the clang-cl the
+        # build is configured with; fetch the run-clang-tidy driver to match.
+        curl -sfLO https://raw.githubusercontent.com/llvm/llvm-project/release/20.x/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+    - name: Configure
+      shell: bash
+      run: |
+        cmake -S . -B tidy-build -G Ninja \
+          -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl \
+          -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DD3D12_SUPPORT=ON
+    - name: Repair compile database for clang-tidy
+      shell: bash
+      run: |
+        # CMake writes the GFXR_*_PATH defines (DLL paths from CMakeLists.txt)
+        # with quoting that clang's Windows command-line tokenizer cannot
+        # re-parse: the whole rest of each compile command, source file
+        # included, is swallowed into one token and clang-tidy fails with
+        # "no input files". Rewrite them in the well-formed escaped-quote
+        # form, and drop the MSVC-only /MP flag, which clang-cl reports as
+        # unused (fatal under /WX).
+        python - <<'EOF'
+        import json
+        import re
+        path = 'tidy-build/compile_commands.json'
+        db = json.load(open(path))
+        def_pat = re.compile(r'-D"(GFXR_\w+)=\\"([^"\\]*)\\"\\"')
+        for entry in db:
+            command = def_pat.sub(r'-D\1=\\"\2\\"', entry['command'])
+            entry['command'] = command.replace(' /MP ', ' ')
+        json.dump(db, open(path, 'w'), indent=1)
+        print('rewrote', len(db), 'compile commands')
+        EOF
+    - name: Run clang-tidy
+      shell: bash
+      run: |
+        # Check only the sources guarded by Windows-specific macros; the rest
+        # of the code is covered by the Linux clang-tidy job.
+        set -f  # keep bash from glob-expanding the [/\\] in the file regexes
+        MACROS='WIN32|_WIN32|VK_USE_PLATFORM_WIN32_KHR|D3D12_SUPPORT|GFXRECON_AGS_SUPPORT'
+        CPPS=$(python .github/scripts/guarded_sources.py "$MACROS")
+        HDRS=$(python .github/scripts/guarded_sources.py "$MACROS" --headers)
+        test -n "$CPPS"  # an empty list would silently check nothing
+        echo "Checking $(printf '%s\n' "$CPPS" | wc -l) guarded sources"
+        python run-clang-tidy.py -p tidy-build -quiet \
+          -clang-tidy-binary "C:/Program Files/LLVM/bin/clang-tidy.exe" \
+          -warnings-as-errors='*' \
+          -header-filter="$HDRS" \
+          $CPPS
+
   linux:
     name: ${{ matrix.config.name }}
     runs-on: ${{ matrix.config.os }}

framework/decode/dx12_replay_consumer_base.cpp

@@ -495,7 +495,7 @@ void Dx12ReplayConsumerBase::ApplyBatchedResourceInitInfo(
                 {
                     GFXRECON_LOG_WARNING(
                         "Initializing Swapchain Buffers. The before state supposed to be COMMON|PRESENT, but it's %s",
-                        util::ToString(state.states));
+                        util::ToString(state.states).c_str());
                 }
             }
 

framework/encode/dx12_state_tracker.cpp

@@ -646,9 +646,9 @@ void Dx12StateTracker::TrackPresentedImages(IDXGISwapChain_Wrapper*        wrapp
         wrapper_info->image_acquired_info[image_index].is_present_parameters = true;
         wrapper_info->image_acquired_info[image_index].dirty_rects.resize(present_parameters->DirtyRectsCount);
         util::platform::MemoryCopy(wrapper_info->image_acquired_info[image_index].dirty_rects.data(),
-                                   present_parameters->DirtyRectsCount * sizeof RECT,
+                                   present_parameters->DirtyRectsCount * sizeof(RECT),
                                    present_parameters->pDirtyRects,
-                                   present_parameters->DirtyRectsCount * sizeof RECT);
+                                   present_parameters->DirtyRectsCount * sizeof(RECT));
         wrapper_info->image_acquired_info[image_index].scroll_rect   = *present_parameters->pScrollRect;
         wrapper_info->image_acquired_info[image_index].scroll_offset = *present_parameters->pScrollOffset;
     }

framework/encode/dx12_state_tracker.h

@@ -52,7 +52,7 @@ class Dx12StateTracker
 
     template <typename ParentWrapper>
     void AddEntry(REFIID                          riid,
-                  typename void**                 new_handle,
+                  void**                          new_handle,
                   format::ApiCallId               create_call_id,
                   ParentWrapper*                  create_object_wrapper,
                   const util::MemoryOutputStream* create_parameter_buffer)
@@ -74,7 +74,7 @@ class Dx12StateTracker
 
     // Specialize templated AddEntry for API calls (which do not have a calling object).
     void AddEntry(REFIID                          riid,
-                  typename void**                 new_handle,
+                  void**                          new_handle,
                   format::ApiCallId               create_call_id,
                   void*                           create_object_wrapper,
                   const util::MemoryOutputStream* create_parameter_buffer)

framework/generated/dx12_generators/dx12_add_entries_header_generator.py

@@ -63,7 +63,7 @@ def endFile(self):
 
         write('template <typename ParentWrapper>', file=self.outFile)
         write(
-            'const std::unordered_map<IID, std::function<void(typename void**, format::ApiCallId, ParentWrapper*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryFunctionTable',
+            'const std::unordered_map<IID, std::function<void(void**, format::ApiCallId, ParentWrapper*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryFunctionTable',
             file=self.outFile
         )
         write('{', file=self.outFile)
@@ -83,7 +83,7 @@ def endFile(self):
         self.newline()
 
         write(
-            'const std::unordered_map<IID, std::function<void(typename void**, format::ApiCallId, void*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryVoidFunctionTable',
+            'const std::unordered_map<IID, std::function<void(void**, format::ApiCallId, void*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryVoidFunctionTable',
             file=self.outFile
         )
         write('{', file=self.outFile)
@@ -202,7 +202,7 @@ def generate_all(self):
         self.newline()
 
         decl = 'template <typename Wrapper, typename ParentWrapper>\n'
-        decl += 'void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, ParentWrapper* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)\n'
+        decl += 'void AddEntry(void** new_handle, format::ApiCallId create_call_id, ParentWrapper* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)\n'
         decl += '{\n'
         decl += '    assert(create_object_wrapper != nullptr);\n'
         decl += '    if (*new_handle != nullptr)\n'
@@ -219,7 +219,7 @@ def generate_all(self):
         decl += '}\n'
         decl += '\n'
         decl += 'template <typename Wrapper>\n'
-        decl += 'void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, void* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)\n'
+        decl += 'void AddEntry(void** new_handle, format::ApiCallId create_call_id, void* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)\n'
         decl += '{\n'
         decl += '    assert(create_object_wrapper == nullptr);\n'
         decl += '    if (*new_handle != nullptr)\n'

framework/generated/generated_dx12_add_entries.h

@@ -34,7 +34,7 @@ GFXRECON_BEGIN_NAMESPACE(gfxrecon)
 GFXRECON_BEGIN_NAMESPACE(encode)
 
 template <typename Wrapper, typename ParentWrapper>
-void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, ParentWrapper* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)
+void AddEntry(void** new_handle, format::ApiCallId create_call_id, ParentWrapper* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)
 {
     assert(create_object_wrapper != nullptr);
     if (*new_handle != nullptr)
@@ -51,7 +51,7 @@ void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, Pare
 }
 
 template <typename Wrapper>
-void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, void* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)
+void AddEntry(void** new_handle, format::ApiCallId create_call_id, void* create_object_wrapper, const util::MemoryOutputStream* create_parameter_buffer, std::mutex &state_table_mutex, Dx12StateTable &state_table)
 {
     assert(create_object_wrapper == nullptr);
     if (*new_handle != nullptr)
@@ -69,7 +69,7 @@ void AddEntry(typename void** new_handle, format::ApiCallId create_call_id, void
 
 
 template <typename ParentWrapper>
-const std::unordered_map<IID, std::function<void(typename void**, format::ApiCallId, ParentWrapper*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryFunctionTable
+const std::unordered_map<IID, std::function<void(void**, format::ApiCallId, ParentWrapper*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryFunctionTable
 {
     { IID_ID3D12RootSignature, AddEntry<ID3D12RootSignature_Wrapper, ParentWrapper> },
     { IID_ID3D12RootSignatureDeserializer, AddEntry<ID3D12RootSignatureDeserializer_Wrapper, ParentWrapper> },
@@ -221,7 +221,7 @@ const std::unordered_map<IID, std::function<void(typename void**, format::ApiCal
     { IID_IDXGIFactory7, AddEntry<IDXGIFactory_Wrapper, ParentWrapper> },
 };
 
-const std::unordered_map<IID, std::function<void(typename void**, format::ApiCallId, void*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryVoidFunctionTable
+const std::unordered_map<IID, std::function<void(void**, format::ApiCallId, void*, const util::MemoryOutputStream*, std::mutex &state_table_mutex, Dx12StateTable &state_table)>,IidHash> kAddEntryVoidFunctionTable
 {
     { IID_ID3D12RootSignature, AddEntry<ID3D12RootSignature_Wrapper> },
     { IID_ID3D12RootSignatureDeserializer, AddEntry<ID3D12RootSignatureDeserializer_Wrapper> },