#3831 Regression for 6502 in SDCC 4.5.0

[Michel Iwaniec]

The newly released SDCC 4.5.0 is giving me pretty severe regressions in BB Studio, my NES fork of the popular GB Studio. (which uses GBDK/SDCC)

I've debugged what is happening in more detail, and it turns out that for re-entrant functions, a very specific code sequence of conditionals paired with another function call is causing the pointer forwarded to the inner function to be wrong: Its lower byte will contain the same value as its upper byte when passed on to the inner function.

Comparing the bytecode, it seems that the culprit is an additional "lda" that clobbers the A register by loading the high byte instead of keeping the low byte. I am not sure if this is a failure due to a stack mismatch, or some other form of accidental clobbering in the codegen for 6502. In either case, the extra code in 4.5.0 looks both redundant and erroneous.

I've extracted the code from the GB Studio game engine and tried to simplify it as much as I can while still triggering the bug. Please see attached file bug-6502.c

This test passes and behaves as intended with SDCC 4.4.0 (#14652), but fails in SDCC 4.5.0 (#15248)

[Philipp Klaus Krause]

I guess part of the problem is that we never enabled the uc6502-stack-auto (i.e. --mmos6502 --stack-auto) regression test target int eh farm, since there were always failures. I currently see on my amd64 Debian GNU/Linux testing system:

Summary for 'uc6502-stack-auto': 5 abnormal stops ( ), 27 failures, 31610 tests, 6017 test cases, 16145276 bytes, 1298356629 ticks

@gorlik, could we get the number of failures down to 7 or less in the next few month? Then we could temporarily disable these tests as known failing for mos6502-stack-auto, open bug tickets for them, and include the uc6502-stack-auto target in the nightly regression testing in the farm. That should give us better protection from introducing mos6502-specific --stack-auto regressions.

[Gabriele Gorla]

The register is being clobbered by an incorrect (and in the specific case unnecessary) sign extension.
Fixed with [r15263]

[Philipp Klaus Krause]

I just tried the version with the fix (but didn't rebuild the library). At first sight, the regression test results look worse:

Summary for 'uc6502-stack-auto': 44 abnormal stops ( ), 2959 failures, 32762 tests, 5959 test cases, 16117051 bytes, 5707955769 ticks

So apparently, the fix broke something affecting many tests cases.

P.S.: We also want a regression test from this bug report. I can take care of that part.

P.P.S: Just saw your second commit now. Will test that one now.

[Philipp Klaus Krause]

Results with the second commit look good for me. In particular some failures in the existing regression tests were fixed by it.

Summary for 'uc6502': 0 failures, 32660 tests, 6024 test cases, 9663466 bytes, 368578066 ticks

Summary for 'uc6502-stack-auto': 5 abnormal stops ( ), 5 failures, 31589 tests, 6017 test cases, 16145514 bytes, 1302228515 ticks
   abnormal stop: gcc-torture-execute-931102-1.c
   abnormal stop: gcc-torture-execute-931102-2.c
   abnormal stop: gcc-torture-execute-pr28982a.c
   abnormal stop: gcc-torture-execute-pr33870-1.c
   Failure: gcc-torture-execute-pr51466.c
   Failure: longjmp.c
   Failure: muldiv_type_long_storage_none_attr_volatile
   Failure: muldiv_type_long_storage_static_attr_volatile
   abnormal stop: shifts3_type_long

[Michel Iwaniec]

Thank you for addressing this so quickly!

I can confirm the fix in r15263 resolves all the problems I've observed in the BB Studio builds

Our GBDK-2020 cross-platform tests all run fine too. As does the NES port of the GB "Black Castle".

So looks like everything's good to go for a GBDK upgrade.

[Michel Iwaniec]

By the way, could we also add the bug-6502.c file (renamed to bug-3831.c) to sdcc/support/regression/tests ?

This would make it part of the test suite, and prevent the bug from re-appearing in the future.

[Philipp Klaus Krause]

Yes, I wanted to do it yesterday, but was too busy with other stuff. I'm running a few tests right now to see if I need to make any changes to the test, and will commit it later today.

P.S.: Done in [r15265].