stardot / beebasm

A portable 6502 assembler with BBC Micro style syntax
http://www.retrosoftware.co.uk/wiki/index.php/BeebAsm
GNU General Public License v3.0
83 stars 26 forks source link

Issue #66 - Handle out of range integer conversions #69

Closed mungre closed 1 year ago

mungre commented 2 years ago

This is a fix for #66 following on from discussion in #68.

chriskillpack commented 2 years ago

@mungre Works on my M1 Laptop

$ git co upstream/mungre/integer-range
$ cd src
$ make clean && make code
$ cd ..
$ python3 test/testrunner.py
SUCCESS: beebasm tests succeeded

One question - this PR doesn't include my assert fix but all the assert tests passed. How does this branch fix the -1 -> 0 on casting issue I encountered?

mungre commented 2 years ago

@chriskillpack Thank you very much!

ASSERT used to work on x86 because the "double to unsigned int" and "double to signed int" conversions both do a signed conversion internally †. So a TRUE value of -1.0 was converted to an integer -1, which is 4294967295 when interpreted as an unsigned int. It isn't zero so ASSERT did what you'd expect.

ASSERT didn't work on ARM because ARM does an actual unsigned conversion with clamping and -1.0 is clamped to zero, which is FALSE.

After this fix the double to int conversion works consistently on both platforms, resembling the x86 behaviour. -1.0 is converted to -1 in ConvertDoubleToInt and then to 4294967295 in EvaluateExpressionAsUnsignedInt. So ASSERT works the same on both platforms.

However, ASSERT now has a range check (in ConvertDoubleToInt) which it lacked previously. As Steve said, it would make more sense for ASSERT just to use a double rather than to convert to an int or to an unsigned int. You could update your ASSERT fix to use doubles which would eliminate the range check.

† I've got the details wrong twice now; third time lucky! The VS2010 compiler produces quite different code for the signed and unsigned conversions even though they do nearly the same thing. For signed conversions cvttsd2si is used to create a 32-bit result, or 0x80000000 if the number is out of range. This can't be used for unsigned conversions (because e.g. 4294967295 is out of range) so fistp is used to create a signed 64-bit result, which is then truncated to 32 bits. So unsigned conversions do work mod 2^32 but this seems to be a compiler implementation detail because x86 seems to lack a direct conversion from double to unsigned integer.

chriskillpack commented 1 year ago

@mungre What additional testing is required before you feel confident enough to merge this into proposed-updates?

mungre commented 1 year ago

Sorry, I didn't merge it immediately in case there were further comments but then I forgot about it. It's done now.