search

jrs0ul / nes_survival

A survival game for the NES
19 stars 2 forks source link

Memory Assignment Overlap #61

Closed eightbithero closed 2 months ago

eightbithero commented 2 months ago

I am writing a utility to check for address allocation conflicts and range overlap for variables during manual allocation in ca65/ld65. I have currently found the following conflicts in the project that can lead to undesired logic execution:

ZEROPAGE

al 0000EE .FAMISTUDIO_DPCM_PTR
al 0000EE .InCave
al 00001E .FAMISTUDIO_SFX_CH2
al 00001E .AnimalSpawnPointsPtr
al 000024 .ItemUpdateDelay
al 000024 .CutsceneDelay
jrs0ul commented 2 months ago

Interesting. It looks like only the second overlap might do some damage. Will your utility be available to the public?

eightbithero commented 2 months ago

Of course it will be in the public domain. At the moment the byte counting is not implemented, as in the design for example 

FOO_BAR: .res 10

reassignment 

SOME_VAR = FOO_BAR

or explicit allocation

FOO_BAZ  = $0011

Written in golang but can be ported to C

eightbithero commented 2 months ago

Quick update, because i just added sourcecode scan for .res XX 

00000E - 00000F TextPtr
00000F - 00000F FAMISTUDIO_SFX_CH1

00001E - 00001E FAMISTUDIO_SFX_CH2
00001E - 00001F AnimalSpawnPointsPtr

000024 - 000024 CutsceneDelay
000024 - 000024 ItemUpdateDelay

00002A - 000049 RamPalette
00002D - 00002D FAMISTUDIO_SFX_CH3

0000EE - 0000EE InCave
0000EE - 0000EE FAMISTUDIO_DPCM_PTR
eightbithero commented 2 months ago

Quick update #2

I have take a look at famistudio library famistudio_ca65.s

.define FAMISTUDIO_CA65_ZP_SEGMENT   ZEROPAGE
.define FAMISTUDIO_CA65_RAM_SEGMENT  BSS
.define FAMISTUDIO_CA65_CODE_SEGMENT ROM6
...

 FAMISTUDIO_CFG_SFX_SUPPORT   = 1 
 FAMISTUDIO_CFG_SFX_STREAMS   = 2
...

.if FAMISTUDIO_CFG_SFX_SUPPORT
    FAMISTUDIO_SFX_STRUCT_SIZE = 15

    FAMISTUDIO_SFX_CH0 = FAMISTUDIO_SFX_STRUCT_SIZE * 0
    FAMISTUDIO_SFX_CH1 = FAMISTUDIO_SFX_STRUCT_SIZE * 1
    FAMISTUDIO_SFX_CH2 = FAMISTUDIO_SFX_STRUCT_SIZE * 2
    FAMISTUDIO_SFX_CH3 = FAMISTUDIO_SFX_STRUCT_SIZE * 3
.endif

and by now hardcoded logic that if variable FAMISTUDIO_SFX_CH its size 15 bytes

so updated result of overlapping is

Overlap detected:

000000 - 00000E FAMISTUDIO_SFX_CH0
000000 - 000000 famistudio_r0
000001 - 000001 famistudio_r1
000002 - 000002 famistudio_r2
000003 - 000003 famistudio_r3
000004 - 000004 famistudio_ptr0
000006 - 000006 famistudio_ptr1
000008 - 000008 current_bank
000009 - 000009 oldbank
00000A - 00000A bankBeforeStatusBarLoad
00000B - 00000B bankBeforeNMI
00000C - 00000D pointer
00000E - 00000F TextPtr

00000F - 00001D FAMISTUDIO_SFX_CH1
000010 - 000011 DigitPtr
000012 - 000013 MapPtr
000014 - 000015 IntroSpritePtr
000016 - 000017 IntroSpriteCoordPtr
000018 - 000019 pointer2
00001A - 00001B PalettePtr
00001C - 00001D CurrentMapPalettePtr

00001E - 00002C FAMISTUDIO_SFX_CH2 <- this will not affect due-to only 2 sfx channels by configuration
00001E - 00001F AnimalSpawnPointsPtr
000020 - 000021 ProjectilePtr
000022 - 000022 tmpAttribAddress
000023 - 000023 InputUpdateDelay
000024 - 000024 CutsceneDelay
000024 - 000024 ItemUpdateDelay
000025 - 000025 NpcAIUpdateDelay
000026 - 000026 NpcCollisionDelay
000027 - 000027 StaminaDelay
000028 - 000028 NpcEliminationDelay
000029 - 000029 PaletteUpdateSize

00001E - 00002C FAMISTUDIO_SFX_CH2  <- this will not affect due-to only 2 sfx channels by configuration
00002D - 00003B FAMISTUDIO_SFX_CH3  <- this will not affect due-to only 2 sfx channels by configuration
00002A - 000049 RamPalette

0000EE - 0000EE FAMISTUDIO_DPCM_PTR
0000EE - 0000EE InCave

It mean if all 2 sfx will be used at once, it may corrupt data a lot

I like how this done in famitone5 by nesdoug https://github.com/nesdoug/famitone5.0 he add output that tell exactly how much memory it takes to use audio-library

famitone5.s

.out .sprintf("last FT variable at %x", LAST_FT)
.out .sprintf("safe to use address at %x", POST_FT)
eightbithero commented 2 months ago

https://github.com/eightbithero/v_alloc - my utility

jrs0ul commented 2 months ago

Awesome, thank you, will try to do something about it

eightbithero commented 2 months ago 
[17:35:17]:nes_survival $ git diff src/famistudio_ca65.s
diff --git a/src/famistudio_ca65.s b/src/famistudio_ca65.s
index 1ea18a1..eb4adf0 100644
--- a/src/famistudio_ca65.s
+++ b/src/famistudio_ca65.s
@@ -1048,10 +1048,6 @@ famistudio_ptr1_hi = famistudio_ptr1+1
 .endif
 .export famistudio_sfx_init
 .export famistudio_sfx_play
-.exportzp FAMISTUDIO_SFX_CH0
-.exportzp FAMISTUDIO_SFX_CH1
-.exportzp FAMISTUDIO_SFX_CH2
-.exportzp FAMISTUDIO_SFX_CH3
 .endif
 .if FAMISTUDIO_USE_DPCM_BANKSWITCHING
 .global famistudio_dpcm_bank_callback

Just remove this from src/famistudio_ca65.s will go open ticket in famistudio repo

And after that everything seems ok

000024 - 000024 CutsceneDelay
000024 - 000024 ItemUpdateDelay

0000EE - 0000EE FAMISTUDIO_DPCM_PTR
0000EE - 0000EE InCave
eightbithero commented 2 months ago

https://github.com/BleuBleu/FamiStudio/issues/340 jfyi it was fun. Good luck with your project

eightbithero commented 2 months ago

It looks like these constants (their values) are needed for the engine to work correctly, but since in the nes_survival code they are available within one file and do not require importing/exporting a symbol between modules, deletion will work.

I am looking for a suitable solution for famistudio_ca65 driver developers to pass these constants as values instead of addresses

jrs0ul commented 2 months ago

You're doing awesome work! (Also motivating me to be less lazy with my project :)) I didn't see any issues after commenting out those variables in the famistudio file, all sound effects surprisingly still works, but now I will know were to look if something happens. Thanks!