Skip to content

Fix array index out-of-bounds risk in comb_repeater.sv by restructuring generate blocks #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

JacyCui
Copy link
Contributor

@JacyCui JacyCui commented Jul 20, 2025

Description

This PR fixes a ​​compile-time array index out-of-bounds risk​​ in comb_repeater.sv by moving conditionals ​​outside​​ always_comb blocks within the generate loop.

Problem

The original code uses always_comb with internal if-else to select between in and s2[i+1] for s1[i]:

always_comb begin
    if( i==(LENGTH-1) ) begin
        s1[i][WIDTH-1:0] <= ~in[WIDTH-1:0];  // Safe
    end else begin
        s1[i][WIDTH-1:0] <= ~s2[i+1][WIDTH-1:0];  // OOB risk if `i=LENGTH-1` (but prevented by if-else)
    end
end

While functionally correct (due to the if-else guard), this structure forces the compiler to ​​statically check​​ s2[i+1] even when i==LENGTH-1, which could theoretically trigger tool errors or synthesis issues.

Fix​​

Restructure to ​​prune invalid paths at compile time​​ by moving conditionals into the generate block:

if( i==(LENGTH-1) ) begin
    always_comb begin
        s1[i][WIDTH-1:0] <= ~in[WIDTH-1:0];  // Only generated for i=LENGTH-1
    end
end else begin
    always_comb begin
        s1[i][WIDTH-1:0] <= ~s2[i+1][WIDTH-1:0];  // Only generated for i<LENGTH-1
    end
end

Impact

  • ​​No functional change​​: Logic behavior is identical.
    ​- ​Safer compilation​​: Eliminates any tool-specific warnings about potential OOB accesses.
    ​​- Better synthesis​​: Tools will not see unused array indices (e.g., s2[LENGTH]).

Additional Context

This follows the same pattern as the fix for adder_tree.sv (PR #4), where generate-level conditionals are safer than always_comb-internal branching.

@pConst
Copy link
Owner

pConst commented Aug 2, 2025

Hi! There is no point to fix "potential" and "theoretical" errors.
I suspect your PRs are GPT-generated. If so, please don`t bother me with this shit anymore

@pConst pConst closed this Aug 2, 2025
@JacyCui
Copy link
Contributor Author

JacyCui commented Aug 3, 2025

Hi, thanks for your reply—I truly appreciate your contributions to this project.

I understand your perspective, and I agree that some issues may seem purely theoretical at first glance. That said, Verilog/SystemVerilog is a notoriously complex language, and real-world toolchains often diverge subtly from the spec. In our experience, writing more defensively around constant index expressions improves portability and avoids surprises across different simulators and synthesis tools.

In this particular case, I actually encountered a real compile-time error during development. Our internal toolchain performs stricter static checks and flagged the original code for potential constant index out-of-bounds—even though the branch is unreachable at runtime. The fix in this PR resolved the issue in our setup, which is why I proposed it. Unfortunately, I’m unable to share a reproducible artifact due to internal restrictions.

Lastly, I sincerely apologize if the PR came off as intrusive or annoying—that was never my intention. I simply wanted to share a fix that helped in my own use case.

Thanks again for your time and for maintaining the project.

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.

2 participants