MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
556 stars 121 forks source link

Maximum negative value for -Wint-overflow off by one? #817

Closed jrudess closed 9 months ago

jrudess commented 9 months ago

I think there is an off-by-one issue for -Wint-overflow, but I couldn't find any discussion of the boundary values in the LRM.

I was expecting a signed variable to allow values ranging from -2^31 to 2^31-1, but it looks like the int-overflow check is only allowing -2^31+1 to 2^31-1

slangtest144.sv:5:18: warning: signed integer literal overflows 32 bits, will be truncated to -2147483648 [-Wint-overflow]
        int b = -2147483648;
                 ^
class C;

    function f();
        int a = -2147483647;
        int b = -2147483648;
    endfunction

endclass
jrudess commented 9 months ago

Just saw that H.7.4 does map SV data types to C types and found LONG_MIN to be -2147483647 in the standard.

jrudess commented 9 months ago

In the C definition, INT_MIN is -32767, but neither of the following give a warning. I increased the value to be an even larger negative number and still didn't get a warning. Is the literal value always being assumed to be 32-bit?

class C;

    function f();
        shortint a = -32768;
        shortint b = shortint'(-32768);
    endfunction

endclass
MikePopoloski commented 9 months ago

So there's a few things happening here. First, literal integers without a base are always 32-bits (or larger; the LRM 5.7.1 says "The number of bits that make up an unsized number (which is a simple decimal number or a number with a base specifier but no size specification) shall be at least 32.") Regardless of where it's used or what's on the lhs of an assignment, the literal itself has that type.

Second, there isn't really such a thing as a negative literal integer; the minus sign is the minus operator applied to the positive literal. That's why you're getting that warning; the lexer produces the integer token and sees that it overflows the positive bounds. The minus sign then gets applied to that overflowed value, but in two's complement negating the most negative integer like that just produces the same most negative value, so you end up with the right result anyway. I could add something to suppress the warning in this case because it's kind of unhelpful, but VCS does issue the same warning so maybe it's helpful to have parity?

Warning-[DCTL] Decimal constant too large
testbench.sv, 5
  Decimal constant '2147483648' is too large as a 32bit signed constant, it 
  should be smaller than 2147483648.
  VCS is using converted signed value '-2147483648' instead.
  To make it an unsigned constant, please use sized constant format.

For the shortint case, you don't get the same warning because the integer literal is still a signed 32-bit value and it fits comfortably. You should get a Wconstant-conversion warning when assigned to a shortint because the value gets truncated, but for some reason that's not working. I'll fix that.

Finally, the C definitions for INT_MIN / LONG_MIN in specification only provide min values (that are symmetric with their positive values, in case an implementation doesn't use two's complement I guess). Actual implementations use values that match the real bounds of their types, for example with gcc on x86_64 INT_MIN is -2147483648 as you'd expect for a 32-bit signed integer. I think this detail is mostly irrelevant as far as SystemVerilog types are concerned, Annex H is just about DPI interop.

MikePopoloski commented 9 months ago

Also LRM 11.3.3 has more light on this subject:

int IntA;
IntA = -12 / 3;      // The result is -4
IntA = -'d 12 / 3;   // The result is 1431655761
IntA = -'sd 12 / 3;  // The result is -4
IntA = -4'sd 12 / 3; // -4'sd12 is the negative of the 4-bit
                     // quantity 1100, which is -4. -(-4) = 4
                     // The result is 1
jrudess commented 9 months ago

Thanks for the references.

jrudess commented 9 months ago

Using -32'sd214783648 does resolve the Slang (and presumably VCS) warning, but the shortint case with an explicit 16-bit signed format doesn't trigger the warning. Would you expect it to?

e.g.

shortint a = -16'sd32769;

MikePopoloski commented 9 months ago

Yes indeed, there's a warning -Wvector-overflow which I think should trigger in this case but it's not considering the sign bit when checking.

MikePopoloski commented 9 months ago

Issues in this thread should be fixed now.