search

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

ASSERT directive not working #66

Closed chriskillpack closed 1 year ago

chriskillpack commented 2 years ago

Compiled locally on Mac OSX against 77609cc67880fd7cc61ef6875a00dcb20ee8a9bd. The assertdemo.6502 example fails to assemble:

$ ./beebasm -i examples/assertdemo.6502
examples/assertdemo.6502:4: error: Assertion failed.

 assert (addr_offset + 3) <= 255

From my own experimentation assert 1 works but assert -1 fails. The issue appears to be that expressions that are true evaluate to -1 which is not considering to be true by assert. PRINT TRUE will output -1.

mungre commented 2 years ago

There have been many updates since then in the proposed-updates branch. These include a bug fix where a parse failure quietly returned zero, which might explain what you are seeing. Can you try it with the more recent code?

chriskillpack commented 2 years ago

Hi, I tried that branch but the issue remains. I've been instrumenting the assembler and I think I've narrowed the issue down to the unsigned int cast in LineParser::EvaluateExpressionAsUnsignedInt. If LineParser::EvaluateExpression returns -1 then the cast clamps that to 0 which HandleAssert treats as a failed assert. Example of this casting behavior and how it doesn't happen with signed int, https://godbolt.org/z/WxMa79zr3. I think the unsigned int cast may be Undefined Behavior. Edit: the pulled quotes from the C spec in https://stackoverflow.com/a/4752947 confirm this is UB.

With this finding, a workaround is to multiply the result of the assert expression by -1 which forces LineParser::EvaluateExpression to return 1, and then asserts behave as expected:

assert -1*(65==65)  \ Passes
assert -1*(65==64)  \ Fails

One minor comment is that the first assert in assertdemo.6502 should fail because addr_offset + 3 == 257. However on first glance and with no comment I would expect all the asserts to pass. Or at least a failing assert should come after passing asserts. I can submit a PR to make those changes to this example.

chriskillpack commented 2 years ago

PR with fix https://github.com/stardot/beebasm/pull/67

mungre commented 2 years ago

Thanks for diagnosing that.

I should have said, beebasm has a test suite now in the test subdirectory, with instructions there. It seems like the "1-values/values.6502" test should fail on your ARM Mac.

assertdemo also appears as "test/5-errors/assertdemo.fail.6502" (where the fail.6502 extension indicates it is expected to fail). I agree that it makes sense to add some comments and put the passing tests first. Keeping the versions in examples and test the same would probably help avoid confusion too.

Thanks for the pr. I've had a quick look. It needs to be on the proposed-updates branch because that's where everything is happening. Also, things like "ASSERT 1" should succeed, so the test for failure should be "== 0" rather than "!= -1".

chriskillpack commented 1 year ago

I believe this is now resolved, closing.