10000 Fix crash when comparing queue to scalar (#5570) by hankhsu1996 · Pull Request #5950 · verilator/verilator · GitHub
[go: up one dir, main page]
Skip to content

Fix crash when comparing queue to scalar (#5570) #5950

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.

Sign up for GitHub

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 10000 to your account

Jump to bottom
Merged
merged 16 commits into from
May 20, 2025
Merged

Fix crash when comparing queue to scalar (#5570) #5950

merged 16 commits into from
May 20, 2025

Conversation

hankhsu1996
Copy link
Contributor

Summary

Fixes #5570.

This PR adds proper validation to reject comparisons between incompatible aggregate types (e.g. queue vs scalar). Previously, Verilator would generate invalid C++ code and crash during compilation, making it hard to debug.

What changed

  • Introduced areAggregatesComparable to recursively verify that aggregate types are structurally comparable.
  • Updated visit_cmp_eq_gt to call this check and emit an early error with useful context.
  • Removed now-redundant legacy slicing checks in expandBiOp. With the new validation, those conditions can no longer be hit.

Notes

This is my first PR. If there are better conventions or places this logic should live, I’m open to feedback. Appreciate any suggestions to improve this!

@wsnyder wsnyder changed the title Fix crash when comparing queue to scalar Fix crash when comparing queue to scalar (#5570) Apr 20, 2025
wsnyder
wsnyder reviewed Apr 20, 2025
View reviewed changes
Copy link
Member
@wsnyder wsnyder left a comment

Choose a reason for hiding this comment

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

Good work, appreciate the fix.

@wsnyder
Copy link
Member
wsnyder commented May 18, 2025

Thanks for updating. Can you please merge master, resolve conflicts. Then in GitHub hit "resolve conversation" for each of the items you think are fixed. I'll then re-review.

@hankhsu1996
Copy link
Contributor Author
hankhsu1996 commented May 19, 2025 •
edited
Loading

I've cleaned up the coding style a bit and added more test cases to cover additional scenarios.

One thing I wanted to mention is, while I initially tried to use similarDTypes as suggested, I found it wasn't a good fit for this use case. It's stricter than what we need for comparisons, especially for unpacked arrays, where it requires the exact same high/low bounds, even though Verilog allows comparisons as long as the element count matches.

That made me realize there's a more fundamental issue. Verilog doesn't really define a concept like similar types. What it defines are type compatibility rules like matching types (LRM 6.22.1), equivalent types (LRM 6.22.2), assignment-compatible types (LRM 6.22.3), and others. In this case, comparisons of aggregate data types should be based on equivalent types.

So for now, I've added an equivalent types check specifically for aggregate data type comparisons to address this bug. There might be a need for a broader refactor in the future to fully align with Verilog's type compatibility rules, but at this point I'm focusing on fixing the immediate issues.

wsnyder
wsnyder reviewed May 19, 2025
View reviewed changes
@hankhsu1996
Copy link
Contributor Author

Fixed

@wsnyder wsnyder merged commit 25cb31c into verilator:master May 20, 2025
36 checks passed
@wsnyder
Copy link
Member
wsnyder commented May 20, 2025

Thanks! There's a pile of issues with very similar lint-rule-missing cases, it would be excellent if you'd consider making pulls for one/some of those too. (It's also possible this fixed pull already fixes some of them).

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

@wsnyder wsnyder wsnyder left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix to lint error on queue compared to int
2 participants
@hankhsu1996 @wsnyder
0