add savedBattlerAttacker and stack for saved target/attacker

[ghoulslash]

Several effects have had issues overwriting gBattlerAttacker/gBattlerTarget in the past and present. We should move away from backing up battler IDs into other id slots by using savetarget/saveattacker. So I added a stack for redundancy. An example of one possible issue would be backing up a battler id to saved battler and then having a chaining effect that also saved a different battler id to saved battler. The top-most call stack would restore the incorrect battler id.

[DizzyEggg]

Good changes, should be merged soon.

[ghoulslash]

Added requisite safety checks. Also added debug warning statements as failsafe checks. Added a final safety check in MOVEEND_CLEAR_BITS to ensure all saved attackers/targets were restored within a given turn. Ready for re review

[mrgriffin]

Also added debug warning statements as failsafe checks.

Good idea! Would it be possible to turn them into errors in tests? I think something like:

#if TESTING
Test_ExitWithResult(TEST_RESULT_ERROR, "BS_RestoreTarget attempting to restore an empty target!");
#else
DebugPrintfLevel(MGBA_LOG_WARN, "BS_RestoreTarget attempting to restore an empty target!");
#endif

With a:

#if TESTING
#include "test/test.h"
#endif

Ought to work. Annoyingly Test_ExitWithResult only exists if TESTING is defined, so we can't avoid the preprocessor...

Maybe in the future we should use AGB_ASSERT for these sorts of things, and then make that responsible for calling Test_ExitWithResult if TESTING is set (and doing the current behavior of either aborting or doing nothing if not, depending on NDEBUG).

[ghoulslash]

Also added debug warning statements as failsafe checks.

Good idea! Would it be possible to turn them into errors in tests? I think something like:

#if TESTING
Test_ExitWithResult(TEST_RESULT_ERROR, "BS_RestoreTarget attempting to restore an empty target!");
#else
DebugPrintfLevel(MGBA_LOG_WARN, "BS_RestoreTarget attempting to restore an empty target!");
#endif

With a:

#if TESTING
#include "test/test.h"
#endif

Ought to work. Annoyingly Test_ExitWithResult only exists if TESTING is defined, so we can't avoid the preprocessor...

Maybe in the future we should use AGB_ASSERT for these sorts of things, and then make that responsible for calling Test_ExitWithResult if TESTING is set (and doing the current behavior of either aborting or doing nothing if not, depending on NDEBUG).

good suggestion. Just pushed these changes. Also added to the checks in MOVEEND_CLEAR_BITS. In my project I use a lot of asserts, but warnings may be better because assertion fails just shut down mgba. But discussion on that is probably better on discord.
Anyway, ready for review again

[ghoulslash]

This keeps failing with [Test_ExitWithResult](error: implicit declaration of function 'Test_ExitWithResult' [-Werror=implicit-function-declaration]) but builds locally

[AlexOn1ine]

This keeps failing with [Test_ExitWithResult](error: implicit declaration of function 'Test_ExitWithResult' [-Werror=implicit-function-declaration]) but builds locally

It doesn't build with make -jN check. I think you forgot to include the test runner in battle scripts commands

[ghoulslash]

This keeps failing with [Test_ExitWithResult](error: implicit declaration of function 'Test_ExitWithResult' [-Werror=implicit-function-declaration]) but builds locally

It doesn't build with make -jN check. I think you forgot to include the test runner in battle scripts commands

Even after adding that make check fails

[AlexOn1ine]

This keeps failing with [Test_ExitWithResult](error: implicit declaration of function 'Test_ExitWithResult' [-Werror=implicit-function-declaration]) but builds locally

It doesn't build with make -jN check. I think you forgot to include the test runner in battle scripts commands

Even after adding that make check fails

My bad, sorry.

[ghoulslash]

Fixed the error by including test/test.h in battle_script_commands.c but make check them bombs with illegal op codes after a while. I think I saw an error with the sticky web/mirror armor test?

[mrgriffin]

Illegal opcodes usually means stack corruption—something overwriting an lr on the stack, which then causes the CPU to branch into non-code memory, (hopefully!) quickly running into bytes which don't represent a legal opcode. If you have a DACS-capable mgba-rom-test (which unfortunately means building it yourself or explicitly specifying a BIOS atm) then as of #3761 you should get an immediate CRASH instead of waiting a minute for a TIMEOUT.

[ghoulslash]

ready for review. I put incorporating into tests on the backburner. Will open a reminder issue upon merging