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

Fix ASSERT directive to work correctly #67

Closed chriskillpack closed 1 year ago

chriskillpack commented 2 years ago

Also cleaned up assertdemo.6502.

I have only been able to test this fix on Mac OS X OS 12.2 with toolchain Apple clang version 12.0.5 (clang-1205.0.22.9) Target: arm64-apple-darwin21.3.0. Since this bug is caused by undefined behavior I'd appreciate if others could test the PR on Windows and MSVC.

chriskillpack commented 2 years ago

@mungre Moving your comments over from the issue. Should ASSERT 1 be treated as TRUE? BBC BASIC appears to define TRUE as -1 which, and this is pure speculation, maybe what beebasm is trying to do as well? If that is true then it seems that treating 1 as TRUE sends mixed messages. WDYT?

I ran testrunner.py with my current changes and it failed

operators.6502:31: error: Assertion failed.

assert(&80000000>>5=-&4000000)
ZornsLemma commented 2 years ago

I'm not @mungre, but FWIW I believe "ASSERT 1" should pass because BBC BASIC (which we're sort of modelling ourselves on) treats anything other than 0 as TRUE. If you try:

IF 1 THEN PRINT "FOO"

at the BASIC prompt, it will print FOO.

Thanks for your work on this, it's really cool to see this being tested on an ARM-based Mac!

mungre commented 2 years ago

Thank you. The assert(&80000000>>5=-&4000000) failure is likely to be the same problem. The code is careful to implement the shifts in a platform independent way, but the conversion from double to int has the same UB. All the places where doubles get converted to ints need to be fixed to call a single routine (or maybe two) that does this consistently in a platform-independent way. Probably the easiest way to do this is to cast to a signed 64-bit int then truncate to an unsigned 32 bit int. Maybe with a range check that converts to zero if the absolute value is too big.

Do you want to do this as part of this fix? Or we can just open an issue.

chriskillpack commented 2 years ago

I'm digging into the failing operators test and so far it does not look like a simple fix - &80000000 = 2147483648 but the value that comes off the value stack for the shift operator is 2147483647. When that gets shifted down the result is 67108863 instead of 67108864.

My gut is that storing as a double has caused the change in value but I've not been able to confirm this hypothesis in toy examples. The easiest thing would be to do delete the failing test but this papers over the issue.

mungre commented 2 years ago

ARM seems to clamp values when converting from floating point to integer.

So a double on the stack with the value 2147483648 will be converted to 2147483647 when it is converted to an int in LineParser::StackTopTwoInts.

chriskillpack commented 1 year ago

I'm closing this PR as the root issue was addressed elsewhere.