Avoid implementation-specific integer conversion in bit shifting tests

[pyrmont]

The following four bit shifting tests using brushift and brshift in test/suite-corelib.janet fail when compiling on ARM (tested on both Apple M1 and Raspberry Pi ARMv7):

https://github.com/janet-lang/janet/blob/7272f43191501c794bd04549dc17379b0c1cd8ea/test/suite-corelib.janet#L31-L36

All the function calls return 32767. As discussed below, I think this is because of undefined behaviour when type casting values outside the 32-bit integer range.

This PR fixes these tests by using operands that are within the 32-bit range.

Discussion

My understanding is that differences between the x64 and ARM instruction sets are causing different values to result when the type casting in the _vm_bitop_immediate and _vm_bitop macros converts a Janet number to a 32-bit integer. (The macros are used by the JOP_SHIFT_RIGHT_UNSIGNED_IMMEDIATE and JOP_SHIFT_RIGHT_UNSIGNED Janet VM instructions.)

https://github.com/janet-lang/janet/blob/7272f43191501c794bd04549dc17379b0c1cd8ea/src/core/vm.c#L150

https://github.com/janet-lang/janet/blob/7272f43191501c794bd04549dc17379b0c1cd8ea/src/core/vm.c#L178

I believe this is because 0x80000000 is outside the range of 32-bit integers and behaviour for this is undefined. See the second paragraph of Section 6.3.1.4 of the C99 standard reads (emphasis added):

When a value of integer type is converted to a real floating type, if the value being converted can be represented exactly in the new type, it is unchanged. If the value being converted is in the range of values that can be represented but cannot be represented exactly, the result is either the nearest higher or nearest lower representable value, chosen in an implementation-defined manner. If the value being converted is outside the range of values that can be represented, the behavior is undefined.

The best explanation of the relevant difference I could find is in 'Hello ARM: Exploring Undefined, Unspecified, and Implementation-defined Behavior in C++':

On the ARM architecture, when a floating-point value is converted to a 32-bit integer, it saturates; that is, it converts to the nearest value that the integer can represent, if the floating-point value is outside the range of the integer. For example, when converting to an unsigned integer, a negative floating-point value always converts to 0, and to 4294967295 if the floating-point value is too large for an unsigned integer to represent. When converting to a signed integer, the floating-point value is converted to -2147483648 if it is too small for a signed integer to represent, or to 2147483637 if it is too large. On x86 and x64 architectures, floating point conversion does not saturate; instead, the conversion wraps around if the target type is unsigned, or is set to -2147483648 if the target type is signed.

Alternatives

It's very possible that the explanation above is wrong (given that it's all based on things I learnt about for the first time today). And perhaps the tests indicate a deeper problem with the C code and the tests should pass. Maybe @zevv knows? If anything is wrong, please feel free to correct as necessary.

[zevv]

I agree: My K&R A6.3 says: "Integer and Floating: When a value of floating type is converted to integral type, the fractional part is discarded; if the resulting value cannot be represented in the integral type, the behavior is undefined. In particular, the result of converting negative floating values to unsigned integral types is not specified.When a value of integral type is converted to floating, and the value is in the representable range but is not exactly representable, then the result may be either the next higher or next lower representable value. If the result is out of range, the behavior is undefined"

[zevv]

Leaves the question: is this something Janet should catch? I assume Janet is to be kept UB-free whenever possible, so janet_unwrap_integer() on out-of-range numbers should probably throw an exception instead?

[zevv]

Interesting fact: this is how Lua use to do the conversion :smiley:

union i_cast {double d; int i[2]};
#define double2int(i, d, t) \
    {volatile union i_cast u; u.d = (d) + 6755399441055744.0; \
    (i) = (t)u.i[ENDIANLOC];}

[pyrmont]

Following on from @zevv's comment, here's a Stack Overflow question explaining this conversion trick in more detail.

[sogaiu]

On a side note, even though brushift is defined like this, due to the function optimzing in the compiler (I think), JOP_SHIFT_RIGHT_UNSIGNED is not executed for brushift by the vm (am?), but rather JOP_SHIFT_RIGHT_UNSIGNED_IMMEDIATE.

At least that's how it looks under gdb and lldb. Though it can also be verified via disasm:

(disasm (fn [] (brushift 0x80000000 16)) :bytecode)
# =>
@[(ldc 1 0) (sruim 0 1 16) (ret 0)]

Here is sruim.

I mention this because some of the tests are testing brushift, but the test does not end up flowing along the part of vm.c one might expect at first.

I don't know that this matters a whole lot but perhaps at some point it might be interesting to try to have tests for specific parts of run_vm in vm.c.

[primo-ppcg]

If I understand correctly, sruim will be used if the second argument is an integer literal between -128 and 127, which can be prevented by using a var binding.

(var n 16)
(disasm (fn [] (brushift 0x80000000 n)) :bytecode)
# ->
@[(ldc 1 0) (ldc 2 1) (geti 2 2 0) (sru 0 1 2) (ret 0)]

[sogaiu]

Interesting -- I guess it's this bit that is relevant. Specifically, that a literal in certain range yields a slot that satisfies:

/* Check if a slot can be coerced to an immediate value */

whereas a var would not, thus avoiding the "optimization".

Thanks for the explanation and demo.

---

On a side note, for direct testing purposes, I went with this.

[bakpakin]

Instead of avoiding the behavior the VM should probably be fixed to make it portable - I don't know if the correct answer is the Lua conversion trick (probably slower on modern CPUs), or checking arguments and making out of bounds values illegal (also slightly slower).

[bakpakin]

Closing as part of https://github.com/janet-lang/janet/commit/b219b146fae97fabdeac87b3c195858d3b0ffb0e