Skip to content

Conversation

@szalpal
Copy link
Member

@szalpal szalpal commented Oct 24, 2025

Category:

Bug fix (non-breaking change which fixes an issue)

Description:

Coverity fixes October 2025

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

Signed-off-by: Michał Szołucha <[email protected]>
@szalpal szalpal marked this pull request as ready for review October 24, 2025 14:35
@szalpal
Copy link
Member Author

szalpal commented Oct 24, 2025

!build

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR applies two defensive programming improvements identified by Coverity static analysis for October 2025. The changes add an assertion before a division operation in GPU kernel launch configuration and eliminate unnecessary object copies in the Python backend tensor feeding path by using const references instead of value copies.

In extract_windows_gpu.cuh, an assertion is added to explicitly verify that max_win_per_input != 0 before using it as a divisor when calculating max_pad_block_x. This reinforces an invariant already established earlier in the function (line 531 asserts max_win_per_input >= 1) and addresses a theoretical division-by-zero finding from static analysis. The assertion provides local documentation of the precondition at the point of use.

In backend_impl.cc, two variables (typed_shape) are changed from auto to const auto & when capturing the result of ConvertShape(). This eliminates unnecessary copies of TensorListShape or TensorShape objects, reducing memory allocations in the tensor feeding code path. Since these variables are used immediately and not modified, the const reference is both safe and more efficient.

Important Files Changed

Filename Score Overview
dali/kernels/signal/window/extract_windows_gpu.cuh 5/5 Added assertion before division to document precondition that max_win_per_input cannot be zero
dali/python/backend_impl.cc 5/5 Changed two local variables to const references to avoid unnecessary shape object copies

Confidence score: 5/5

  • This PR is safe to merge with minimal risk
  • All changes are defensive optimizations that address static analysis findings without altering behavior; the assertion enforces an existing invariant and the const references eliminate unnecessary copies with no logical changes
  • No files require special attention

Sequence Diagram

sequenceDiagram
    participant User
    participant Pipeline
    participant Operator
    participant Workspace
    participant TensorList
    participant Memory
    participant CUDA

    User->>Pipeline: __init__(PipelineParams)
    Pipeline->>Pipeline: Initialize executor, threads, queues
    
    User->>Pipeline: AddOperator(OpSpec)
    Pipeline->>Pipeline: Store operator spec
    
    User->>Pipeline: Build()
    Pipeline->>Operator: InstantiateOperator(OpSpec)
    Operator-->>Pipeline: Return operator instance
    Pipeline->>Pipeline: Build execution graph
    
    User->>Pipeline: SetExternalInput(name, data)
    alt CPU Input
        Pipeline->>TensorList: Create TensorList<CPUBackend>
        TensorList->>Memory: ShareData() or Copy()
    else GPU Input
        Pipeline->>TensorList: Create TensorList<GPUBackend>
        TensorList->>CUDA: cudaMemcpyAsync()
        TensorList->>Memory: ShareData() or Copy()
    end
    
    User->>Pipeline: Run()
    Pipeline->>Pipeline: Prefetch()
    Pipeline->>Workspace: Setup workspace
    Workspace->>TensorList: Add inputs/outputs
    
    loop For each operator
        Pipeline->>Operator: Setup(OutputDesc, Workspace)
        Operator->>Workspace: Get input shapes/types
        Operator->>TensorList: Resize outputs
        Operator-->>Pipeline: Return output descriptors
        
        Pipeline->>Operator: Run(Workspace)
        Operator->>Workspace: Get input tensors
        Operator->>TensorList: Read input data
        alt GPU Operator
            Operator->>CUDA: Launch kernel
            CUDA->>Memory: Process data on GPU
        else CPU Operator
            Operator->>Memory: Process data on CPU
        end
        Operator->>TensorList: Write output data
        Operator-->>Pipeline: Complete
    end
    
    User->>Pipeline: Outputs()
    Pipeline->>Workspace: GetOutputs()
    Workspace->>TensorList: Return output tensors
    Pipeline-->>User: Return tuple of TensorLists
    
    User->>Pipeline: ReleaseOutputs()
    Pipeline->>Memory: Mark outputs as releasable
    
    User->>Pipeline: Shutdown()
    Pipeline->>Operator: Cleanup
    Pipeline->>Memory: Release resources
    Pipeline->>CUDA: Destroy streams
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [37190227]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [37190227]: BUILD PASSED

if (out_win_length > args.window_length && !concatenate) {
int max_pad_block_x = 32;
if (max_win_per_input < 32) {
assert(max_win_per_input != 0);
Copy link
Contributor

@mzient mzient Oct 27, 2025

Choose a reason for hiding this comment

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

The assertion is already in place (line 531) assert(max_win_per_input >= 1); and max_win_per_input is not modified in between. Mark it as a false positive and don't clutter the code with redundant checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree. This coverity issue will reappear, therefore for the sake of making next iterations smoother it's better to have the assertion right there. Additionally, it doesn't harm release build. Therefore I'm keeping it here.

@mzient mzient self-assigned this Oct 27, 2025
Co-authored-by: Michał Zientkiewicz <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The most recent changes apply formatting fixes to dali/python/backend_impl.cc, adjusting the spacing around reference operators (&) to comply with the project's clang-format configuration, which requires PointerAlignment: Left. Specifically, the changes modify two lines (269 and 331) from & typed_shape to &typed_shape, removing the space between the ampersand and variable name. These are cosmetic-only changes that align with the .clang-format file's requirement that pointer/reference symbols attach to the type rather than the variable. The changes are part of a broader Coverity static analysis cleanup batch for October 2025.

Important Files Changed

Filename Score Overview
dali/python/backend_impl.cc 5/5 Formatting-only changes: removed space between & and variable name on lines 269 and 331 to match clang-format style

Confidence score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are purely cosmetic formatting adjustments with no functional impact; they align code style with the project's clang-format configuration (PointerAlignment: Left)
  • No files require special attention

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants