Fix build issues exposed by newer Clang 21 toolchains#6348
Conversation
|
| Filename | Overview |
|---|---|
| dali/operators/image/remap/jitter.cuh | Replaces deprecated <ctgmath> with <cmath>; no math functions from <cmath> are directly invoked in this file but the change is safe and consistent with the other remap headers. |
| dali/operators/image/remap/sphere.h | Replaces <ctgmath> with <cmath>; uses sqrtf() which is correctly provided by <cmath>. |
| dali/operators/image/remap/water.h | Replaces <ctgmath> with <cmath>; uses sinf() and cosf() which are correctly provided by <cmath>. |
| .gitmodules | cvcuda submodule URL changed to a personal fork; discussed in the previous review thread — temporary pending upstream merge. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Clang 21 build failure"] --> B{"Root cause"}
B --> C["ctgmath removed in C++20\n(jitter.cuh, sphere.h, water.h)"]
B --> D["uint8_t undeclared in\nCVCUDA OSD legacy header"]
C --> E["Replace with cmath\n(sufficient — no complex math used)"]
D --> F["Bump cvcuda submodule\nto fork commit 3f8cc441\n(adds #include stdint.h)"]
E --> G["Build succeeds"]
F --> G
Reviews (3): Last reviewed commit: "Fix DALI build with Clang 21" | Re-trigger Greptile
| [submodule "third_party/cvcuda"] | ||
| path = third_party/cvcuda | ||
| url = https://github.com/CVCUDA/CV-CUDA.git | ||
| url = https://github.com/JanuszL/CV-CUDA.git |
There was a problem hiding this comment.
[Question] Permanent fork reference being merged to
main
The PR description calls this change "temporary," but merging it means every developer checkout and every CI job will clone from github.com/JanuszL/CV-CUDA until a follow-up PR reverts it. If the fork is deleted, made private, or diverges further, all builds that depend on this submodule will break with no obvious error message. Personal forks are not appropriate long-term submodule URLs for a shared repository.
Is the upstream fix already submitted to CVCUDA/CV-CUDA? The typical safe path is to land the fix upstream first, then bump the submodule to the official commit, keeping the canonical URL intact. If the upstream PR will land soon, consider holding this change off main until then rather than merging the fork URL.
There was a problem hiding this comment.
Relevant change has been opened in the corresponding project. Until it is included in the official release we need to use form :(
|
!build |
|
CI MESSAGE: [51385562]: BUILD FAILED |
|
CI MESSAGE: [51388370]: BUILD STARTED |
|
CI MESSAGE: [51388370]: BUILD FAILED |
f77c3b3 to
ec2d3fa
Compare
|
CI MESSAGE: [51396198]: BUILD STARTED |
|
CI MESSAGE: [51396198]: BUILD FAILED |
Replace obsolete <ctgmath> includes in the remap headers with <cmath> so they build with newer Clang toolchains. Update the CVCUDA submodule to a fork commit that adds the missing <stdint.h> include to CvCudaOSD.hpp. The legacy OSD header uses uint8_t directly and no longer gets it transitively with the new toolchain include graph. Point the CVCUDA submodule URL to the fork that contains the fix so CI can fetch the updated submodule commit. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
ec2d3fa to
014ffdd
Compare
|
CI MESSAGE: [51399721]: BUILD STARTED |
|
CI MESSAGE: [51399721]: BUILD PASSED |
|
CI MESSAGE: [51600765]: BUILD STARTED |
|
CI MESSAGE: [51601446]: BUILD STARTED |
|
CI MESSAGE: [51600765]: BUILD FAILED |
|
CI MESSAGE: [51601446]: BUILD FAILED |
The DALI remap headers included obsolete <ctgmath>. This PR replaces those includes with <cmath> in the sphere and water remap operators. The CVCUDA legacy OSD header uses uint8_t directly but did not include a header that defines it. Some toolchains no longer provide uint8_t transitively through the current include graph, which makes the CVCUDA build fail with unknown type name 'uint8_t'. This PR updates the CVCUDA submodule to a fork commit that adds the missing <stdint.h> include. The CVCUDA submodule URL is changed to https://github.com/JanuszL/CV-CUDA.git so CI can fetch the fix commit. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
The DALI remap headers included obsolete <ctgmath>. This PR replaces those includes with <cmath> in the sphere and water remap operators. The CVCUDA legacy OSD header uses uint8_t directly but did not include a header that defines it. Some toolchains no longer provide uint8_t transitively through the current include graph, which makes the CVCUDA build fail with unknown type name 'uint8_t'. This PR updates the CVCUDA submodule to a fork commit that adds the missing <stdint.h> include. The CVCUDA submodule URL is changed to https://github.com/JanuszL/CV-CUDA.git so CI can fetch the fix commit. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
Fix build issues exposed by newer Clang 21 toolchains.
The DALI remap headers included obsolete
<ctgmath>. This PR replacesthose includes with
<cmath>in the sphere and water remap operators.The CVCUDA legacy OSD header uses
uint8_tdirectly but did not includea header that defines it. Some toolchains no longer provide
uint8_ttransitively through the current include graph, which makes the CVCUDA
build fail with
unknown type name 'uint8_t'. This PR updates theCVCUDA submodule to a fork commit that adds the missing
<stdint.h>include.
The CVCUDA submodule URL is changed to
https://github.com/JanuszL/CV-CUDA.gitso CI can fetch the fix commit.Additional information:
Affected modules and functionalities:
dali/operators/image/remap/sphere.hdali/operators/image/remap/water.hthird_party/cvcudasubmodule advanced to3f8cc441(
Include stdint in OSD legacy header)..gitmodulesupdated forthird_party/cvcudato usehttps://github.com/JanuszL/CV-CUDA.git.Key points relevant for the review:
<ctgmath>with<cmath>.fixed-width integer include in the header that uses
uint8_t.upstream CVCUDA.
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A