Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arithmetic tests are expecting the wrong result. VM int32, int64, uint64 evaluation does not match runtime. #11138

Closed
genotrance opened this issue Apr 29, 2019 · 6 comments
Assignees
Labels
Severe Test Suite VM see also `const` label

Comments

@genotrance
Copy link
Contributor

Test in tarithm.nim fails on armv6 and armv7 but passes on arm64.

https://github.com/nim-lang/Nim/blob/devel/tests/arithm/tarithm.nim#L129

Expected value is 1 but actual is 0. Tested 1 << 32 on a real RPi and it is 0 there as well. Seems like the test needs to be updated.

@mratsim
Copy link
Collaborator

mratsim commented Apr 30, 2019

The tests are completely wrong and linked to VM bug #9572.

Forcing runtime evaluation, we should get all 0:

proc doShl(T: typedesc[SomeSignedInt], shift: int): T =
    1.T shl shift

block:
  let t0 = doShl(int8, 8)
  let t1 = doShl(int16, 16)
  let t2 = doShl(int32, 32)
  let t3 = doShl(int64, 64)
  echo t0
  echo t1
  echo t2
  echo t3

Merely using a let instead of const won't work because Nim still compute 1 shl 32 at compile-time and inlines it.

Edit:

Unsigned u64 is wrong as well and showstopper for bigint library that want to do compile-time shifts like Stint

# Unsigned types
block:
const t0: uint8 = 1'u8 shl 8
const t1: uint16 = 1'u16 shl 16
const t2: uint32 = 1'u32 shl 32
const t3: uint64 = 1'u64 shl 64
doAssert t0 == 0
doAssert t1 == 0
doAssert t2 == 0
doAssert t3 == 1

Edit 2:
Changed the title to make it clear that the issue is not the ARM target but Nim VM and the tests that are incorrect.

@mratsim mratsim changed the title Arithmetic test fails on armv6 and armv7 Arithmetic test are expecting the wrong result. VM int32, int64, uint64 evaluation does not match runtime. Apr 30, 2019
@mratsim mratsim changed the title Arithmetic test are expecting the wrong result. VM int32, int64, uint64 evaluation does not match runtime. Arithmetic tests are expecting the wrong result. VM int32, int64, uint64 evaluation does not match runtime. Apr 30, 2019
@krux02 krux02 self-assigned this May 6, 2019
@krux02
Copy link
Contributor

krux02 commented May 6, 2019

To be honest, I don't understand these tests. Bitshifting these amount of bits in C is undefined behavior. Since the shl is basically a wrapper for the C bitshifting operator, I don't get why these tests have been written in this way. Neither do I understand the logic behind them, nor do I have any clue on how we can ensure these results in the future.

@Araq
Copy link
Member

Araq commented May 6, 2019

Maybe because Nim is not C and it's defined behavior in Nim? Wasn't that hard to figure it out, was it...

@genotrance
Copy link
Contributor Author

Well the results are inconsistent and blocking megatest on arm nightlies. Please suggest a fix - either when arm or removing these overflow bit-shift tests altogether.

@jangko
Copy link
Contributor

jangko commented May 31, 2019

Looks like the behavior is architecture dependent, see: stackoverflow.

If that is the case, the tests are tied to specific architecture.

If we want to make VM and runtime produce the same result, we should not let it have undefined behavior as in C, but should define the behavior for both VM and runtime in case of shift overflow.

@krux02
Copy link
Contributor

krux02 commented Aug 14, 2019

The tests in discussion have been removed by new, because they were incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Severe Test Suite VM see also `const` label
Projects
None yet
Development

No branches or pull requests

6 participants