Check STF and places headers for escaping exceptions - #10612
Conversation
bugprone-exception-escape is enabled repo-wide, but the clang-tidy job analyzes translation units compiled by clang's CUDA front end, which STF and places are excluded from, so neither was ever checked. Sweep their headers as host C++ instead: an escaping exception is a host-side property, and a host parse needs no CUDA support, so the check now covers STF even where STF compilation is disabled. The sweep found seven sites. A trace file that fails to write is not worth terminating over, so ~dot now reports instead; ~localized_array called cuda_try where a destructor must use cuda_safe_call; meyers_singleton::instance() claimed noexcept unconditionally while constructing a user type, and is now noexcept only when that construction is. The remaining four terminate by design and say so.
|
/ok to test c6b8d4c |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesException-escape linting
Suggested reviewers: Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cudax/include/cuda/experimental/__utility/meyers_singleton.cuh (1)
88-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Add an explicit host/device annotation to
instance().The changed declaration still has no
_CCCL_*_APImarker. Because it creates a function-local static, confirm that the function is host-only and use_CCCL_HOST_API; do not use a host-device annotation unless this implementation is supported by the target CUDA toolchains. As per coding guidelines, functions must be marked with an appropriate_CCCL_*_APIannotation. As per path instructions,cudax/**/*reviews must focus on host/device annotations and supported CUDA toolchain compatibility.Sources: Coding guidelines, Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 426b3d4a-6c91-4357-a987-6c7d7eea3505
📒 Files selected for processing (10)
cmake/CCCLAddTidyTarget.cmakecudax/CMakeLists.txtcudax/cmake/cudaxTidyExceptionEscape.cmakecudax/include/cuda/experimental/__places/localized_array.cuhcudax/include/cuda/experimental/__places/places.cuhcudax/include/cuda/experimental/__stf/internal/dot.cuhcudax/include/cuda/experimental/__stf/internal/logical_data.cuhcudax/include/cuda/experimental/__stf/stackable/stackable_ctx.cuhcudax/include/cuda/experimental/__utility/meyers_singleton.cuhdocs/infrastructure/cmake/references/cmake_modules.rst
Summary
bugprone-exception-escapeis already enabled repo-wide in.clang-tidywithWarningsAsErrors: '*', but it has never seen STF or places: the clang-tidy job analyzes translation units compiled by clang's CUDA front end, andci/build_tidy.shdisables both subprojects there (cudax_ENABLE_CUDASTF=OFF,cudax_ENABLE_PLACES=OFF, per the existingTODO(jfaibussowit)about clang-cuda).Rather than wait on clang-cuda support, this sweeps those headers as host C++. An escaping exception is a host-side property, so ignoring device code loses nothing, and a host parse needs no CUDA support at all — which is what lets the check cover STF while STF compilation there stays disabled. The sweep is therefore included unconditionally.
New
cccl_tidy_add_header_sweep()incmake/CCCLAddTidyTarget.cmakegenerates a one-line translation unit per header and analyzes it with an explicit host C++ command line, so it needs no compilation-database entry and also covers headers no translation unit happens to include. It attaches tocudax.tidy, hence tocccl.tidy, so the existing clang-tidy job picks it up with no CI configuration change.Two details worth knowing:
_CCCL_NO_SYSTEM_HEADER. CCCL headers declare themselves system headers, clang-tidy honors that and drops every diagnostic raised inside them, so without it the sweep silently reports nothing.atomicCAS,threadIdx), andstackable_task_dep.cuhis not self-contained in a host parse but is still analyzed throughstf.cuh.Findings and how they were handled
The check found seven sites, all in destructors or
noexceptfunctions, where an exception already means termination today:~dot()→finish()~localized_array()cuda_try(throws) three times in a destructor; nowcuda_safe_call(reports and aborts), which is the convention for destructors.meyers_singleton::instance()noexceptwhile default-constructing a user type; nownoexceptexactly when that construction is.~logical_data_untyped_impl()→erase()NOLINTNEXTLINE+ rationale: a write-back failing part-way through destruction is not recoverable.~graph_scope_guard(),~state(),launchable_graph_scope::release()NOLINTNEXTLINE+ rationale: an unmatched push or a failed graph teardown is not recoverable.The four annotated sites are the natural home for explicit reporting; a follow-up can replace each annotation with a reporting wrapper rather than leaving the intent implicit.
Test plan
ninja -j 12 cudax.tidy.exception_escape— 104 targets, 43 s wall clock, cleannvccheader tests for every changed header:dot.cuh,logical_data.cuh,stackable_ctx.cuh,localized_array.cuh,meyers_singleton.cuh(7 architectures)noexceptoninstance()verified with g++, clang++ (C++17 and C++20) and nvcc, for both a nothrow and a throwing singleton constructorpre-commit runon all touched files