Fix several open bugs: assertion failures, error reporting, and elaboration issues#1292
Fix several open bugs: assertion failures, error reporting, and elaboration issues#1292apullin wants to merge 12 commits intosteveicarus:masterfrom
Conversation
|
Thank you for all your work on this. I will review this soon and pull all this in before we release 13.0. One note: for #1140 I had assigned this to myself and have mostly completed it thought there are still a couple places I had not finished and there is a proposed patch for #1187. It would have been best if you had asked about these before making changes and if these were individual patches it would make them easier to review and compare to the work others have already done. |
|
@caryr I'll close the PR and resubmit individual per-bug PR's that are non-overlapping. Also this is < 1 day work with Claude Code, I did very little. |
|
Okay, let me review the changes in more detail so I can give some constructive feedback before you submit again. I did notice a couple formatting items, duplicate code, etc. Lets get Claude on the right track from the beginning. Overall things looks good from my initial scan. |
|
For the #1187 change take a look at the #1190 pull request. It looks like that covers all the changes you made and then there were issues with windows. I'm guessing, but I assume the windows changes started getting out of hand and that needed to be resolved in a reasonable manner before this became an official pull request. I'm going to start a discussion regarding the use of AI generated changes in iverilog. Look for that in the discussion area and please contribute. There a a few things I want to make sure we do correct from the beginning. |
|
See #1293 for the AI discussion. |
apullin
force-pushed
the
bugfix/multiple-issues
branch
from
5d30612 to
c5f2ab4
Compare
apullin
force-pushed
the
bugfix/multiple-issues
branch
3 times, most recently
from
a565efb to
87dfed3
Compare
|
Duplicate code for the fix for #1134 was removed. Stack was re-ordered because the tests did fail at one commit, but was then fixed at a later one. Formatting - it seems to be a little inconsistent. My only known solution to this is to adopt a |
|
I'll look at this and comment more once I get the 13.0 release completed. I've been busy with my work and personal commitments so am behind getting this finished. |
When $bits() is called with an undefined identifier, the compiler now properly reports an error instead of silently returning 0. The fix checks if the argument expression has type IVL_VT_NO_TYPE after test_width() processing (indicating the identifier couldn't be resolved), and triggers elaboration to produce a proper error message. Includes regression test: br_gh1112 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Modified grammar in parse.y to use identifier_name instead of IDENTIFIER in function/task declaration rules and hierarchy_identifier rule. This allows TYPE_IDENTIFIER tokens (which class names become after definition) to be used as method names. Changes: - Function declaration rules (lines 1585, 1605, 1631) - Task declaration rules (lines 2445, 2472, 2501) - hierarchy_identifier member access (line 4471) Includes regression test: br_gh670 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The tgt-sizer target now skips IVL_SCT_PACKAGE scopes (the SystemVerilog $unit compilation unit scope) instead of erroring. This allows sizer to work with -g2012 and other SystemVerilog modes. Includes regression test: br_gh1170 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e multiple drivers TODO: Decide how resolved net types (tri0/tri1/triand/trior) should interact with uwire in a shared nexus.
…outputs Per IEEE 1800-2017 6.5, variables can be written by one port, including primitive gate outputs. The code incorrectly disallowed this with the comment "Gates can never have variable output ports." Changed elaborate_lnet/elaborate_bi_net calls for gate outputs to pass true for var_allowed_in_sv, allowing variables as single-driver outputs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
In multi-dimensional packed arrays, allow variable indices in the outer
(prefix) dimensions, not just the final dimension. For example:
logic [3:0][3:0] a;
for (int i=0; i<4; i++)
a[i][3] = 1; // Previously error, now works
The fix checks if any packed prefix indices are non-constant. If so,
use collapse_array_exprs() to compute the bit offset as an expression
rather than requiring constant indices.
This removes an artificial restriction that had no justification in
the IEEE standard, as noted by maintainers in the GitHub issue.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… packed structs When accessing a member of a packed struct within an unpacked array (e.g., `tests[0].a` where `tests` is `test_t tests[0:1]`), the assertion `base_index.size()+1 == net->packed_dimensions()` would fail because unpacked indices were incorrectly included in base_index. The fix: 1. Separate unpacked indices from packed indices before calling check_for_struct_members() 2. Compute the word index for unpacked array access using normalize_variable_unpacked() 3. Pass the word index to check_for_struct_members(), which now creates NetESignal with the proper word selector This allows expressions like `array_of_structs[i].member` to work correctly, selecting the right array element before extracting the packed struct member. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Uwire arrays were triggering assertion failures because they were kept virtualized (pins array NULL) like reg arrays. However, unlike reg arrays, uwire arrays used as input ports need their nexuses initialized for the target code generation to work properly. Two changes: 1. elab_sig.cc: Devirtualize pins for UNRESOLVED_WIRE (uwire) type, same as regular WIRE type. 2. t-dll-api.cc: Update assertion in ivl_signal_nex to also accept IVL_SIT_UWIRE when pins array is NULL. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When an undimensioned (dynamic) array was passed to a task parameter expecting a simple vector, the compiler would crash with an assertion failure because the switch handling type casts didn't know how to handle IVL_VT_DARRAY type. Changed the assertion to emit a proper error message about type incompatibility and continue processing, allowing the compiler to report the error gracefully instead of crashing. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…npacked arrays The original check used `pin_count() > 1` to detect whole-array assignments, which missed single-element arrays like `[0:0]` where pin_count is 1. The fix checks for whole-array assignments by: 1. Multi-element arrays (pin_count > 1) - always whole-array 2. Single-element unpacked arrays - check if the lval expression has array indices. If no indices, it's a whole-array reference. This correctly distinguishes between: - `assign arr = expr` (whole array) -> elaborate_unpacked_array_ - `assign arr[i] = expr` (indexed element) -> normal path Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This bug was fixed by the steveicarus#1265 fix. The error 'Array needs an array index here' no longer occurs for unpacked array literals in continuous assignments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…n confusion This bug was fixed by the steveicarus#1265 fix. Single-element unpacked arrays of packed types (like byte [0:0]) now work correctly in continuous assignments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
apullin
force-pushed
the
bugfix/multiple-issues
branch
from
87dfed3 to
314638a
Compare
2 participants
Summary
This PR fixes 12 bugs from the GitHub issue tracker, focusing on:
Bugs Fixed
Additional Regression Tests
Testing
Each fix includes a regression test in
ivtest/ivltests/added toregress-sv.list.