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 😅

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! 😅

[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.

[martinwhitaker]

Finally found time to review this more thoroughly. Please see comments inline.

[martinwhitaker]

This seems to be unnecessary. The compiler already handles your test case for a constant base correctly. If you compile with -Wall, it will issue an appropriate warning too.

The same goes for the similar changes below.

If you disagree, please provide a test case that fails.

[daglem]

I finally found the time to look at this again with a fresh pair of eyes. I have addressed the comments in your thorough review, and I have cleaned up, corrected, and commented the code to make it more understandable.

As mentioned earlier, if you run the test for constant base through -t vlog95 first, it will fail. I believe it will also fail right away on Windows (LLP64), however I can't test that.

[martinwhitaker]

New tests should be added to the new python-based test framework (see https://steveicarus.github.io/iverilog/developer/regression_tests.html).

[daglem]

OK, done.

[martinwhitaker]

This is correct, but is not easy to understand. I suggest the following:

       /* Calculate the canonical offset. */
      long offset = soff;
      if (msb_lo) {
            offset += lsb;
            if (is_up)
                  offset -= wid - 1;
      } else {
            offset -= lsb;
            if (!is_up)
                  offset -= wid - 1;
      }

[daglem]

OK, done.

[martinwhitaker]

I don't understand the need for this extra expansion. We end up generating an expression to calculate base + offset or offset - base, so where does offset + wid + 1 come into it?

[daglem]

That was due to a misunderstanding on my part. I speculated that the normalized base had to be padded to account for addition of the topmost bit of the selected slice, which evidently is not the case.

[martinwhitaker]

This is only safe if off_max_wid is non-zero and the earlier pad_to_width() has already zero-extended base. If not, the immediately preceding pad_to_width() will sign-extend base. As it happens, I think your extra expansion of off_max_wid guarantees it will never be zero, but if that is removed, you will need to restore the additional NetESelect here to separate the cast from the padding.

[daglem]

It turns out that the NetESelect is necessary to generate the correct %pad/u, otherwise cast_signed will become part of the previous pad_to_width, and %pad/s will be generated.

It basically says so in the comments, I just didn't catch the meaning of it since I didn't know the code base and this somewhat counterintuitive behavior.