Guard against overflow / wrap around of internal bit address

[daglem]

Internally, the maximum address space of a vector is 31 bits + a sign bit to signal invalid addresses (out of bounds or has one or more x or z bits).

This pull request ensures that unsigned parts-select bit addresses which would otherwise overflow and wrap around within this address space are correctly handled as out of bounds.

[daglem]

I should mention that I replaced (two instances of) this code by setting the signing flag directly on the base after widening. I did this since it didn't cause any regressions, and I'm not clear on what it's supposed to achieve.

            /* We need this extra select to hide the signed
             * property from the padding above. It will be
             * removed automatically during code generation. */
          NetESelect *tmp = new NetESelect(base, 0 , min_wid);
          tmp->set_line(*base);
          tmp->cast_signed(true);
                  base = tmp;

Just tell me if there is a good reason why the code should stay, and I'll add it back.

[martinwhitaker]

When I test against the master branch, your test program only fails on the variable base expression. Is that what you expect?

Although this PR appears to fix the problem, the various checks of the form rel_base != (int32_t)rel_base look wrong - rel_base is of type long which is 32 bits on Windows, so those comparisons would always return false.

[daglem]

When I test against the master branch, your test program only fails on the variable base expression. Is that what you expect?

Yes, it only fails there since there was already a bit of checking in place for constants. However if you output Verilog from the test using -t vlog95 and run again on the result, that will fail(!)

Although this PR appears to fix the problem, the various checks of the form rel_base != (int32_t)rel_base look wrong - rel_base is of type long which is 32 bits on Windows, so those comparisons would always return false.

There are exactly two such checks in elab_expr.c, where I commented on that being a possible issue. I guess you're right that overflows won't be caught here on Windows, since it uses the LLP64 data model. I haven't been using Windows since long before the transition to 64 bits, so I had to look that up :sweat_smile:

It is unfortunate that long is used in iverilog instead of a type guaranteed to be at least 64 bits, like e.g. long long or int64_t. To an outsider it would seem that long is expected to be 64 bits in the code - why would the distinction between int and long have been made otherwise? The explanation cannot be that an int could be 16 bits, since that surely would break the code in several places.

Nitpicks aside, I guess I could just add an as_long64() to the verinum class, and use that in the code in question? Since verinum already has as_ulong64(), that seems natural to me.

[martinwhitaker]

I can't speak for Steve's intentions, but bear in mind that Icarus Verilog was created before the x86_64 architecture.

Also, using 64-bit values isn't a panacea. Even with your patches, this bit of code still gives the wrong result:

reg [1:0] array = 2'b01;
reg [63:0] offset = -64'd1;

wire [1:0] outside = array[offset +: 2];

If we are going to fix this, we also need to look at the code that handles the LHS of assignments (elab_lval.cc and elab_net.cc). This may be an opportunity to factor out more code.

[daglem]

Also, using 64-bit values isn't a panacea. Even with your patches, this bit of code still gives the wrong result:

reg [1:0] array = 2'b01;
reg [63:0] offset = -64'd1;

wire [1:0] outside = array[offset +: 2];

As indicated in the comments in the test partsel_outside.v, the intention is to avoid incorrect results when unsized numbers (which are 32 bits in iverilog) are used in the base expression in part selects (this issue was discovered while running the following test, where iverilog will in some cases incorrectly fetch bits from the din vector: https://github.com/YosysHQ/yosys/blob/0909c2ef5efbf51e2ff90f35457612ba8a51a251/tests/simple/partsel.v#L136).

It is outside the scope of this PR to correctly handle 64 bit addresses and beyond - 64 bit numbers are only used to ensure that the part select bit accesses outside of the (current) internal 31 bit address space will return x bits.

If we are going to fix this, we also need to look at the code that handles the LHS of assignments (elab_lval.cc and elab_net.cc). This may be an opportunity to factor out more code.

Making the check for constant base expressions work right in all cases on Windows seems to be quite a bit of work - changing long to int64_t in just one place quickly spreads to several other functions. For now I simply removed the check for constant base expressions in a force push.

I'll see if I can come up with a solution for constant base expressions short of replacing all long with int64_t, in the mean time I believe the PR can be merged in its current form, without introducing any issues.

[daglem]

@martinwhitaker I have now implemented checks for overflow of constant base expressions in elab_expr.cc, elab_lval.cc and elab_net.cc, which you mentioned. These checks will catch 32 bit unsigned -> signed overflow / wrap around as demonstrated in partsel_outside.v. The checks should now also work on Windows! :sweat_smile:

[daglem]

@martinwhitaker I've rewritten the code for variable base expressions to use only 32 bit numbers as well. Now the commit doesn't pretend to solve any other issue than overflow when unsigned 32 bit part select base expressions (resulting from expressions involving unsized numbers in Verilog) are converted to internal signed 32 bit addresses. I think this is now minimally intrusive, and I hope it is more to your liking.