search

RPGHacker / asar

(Now) official repository of the SNES assembler Asar, originally created by Alcaro
Other
204 stars 43 forks source link

Don't removerats twice when two autocleans point to the same freespace #242

Closed spooonsss closed 9 months ago

spooonsss commented 2 years ago

Fixes #216

No test added. Since this bug requires existing RATS tags to repro, I don't think the current testing framework would support a test for this.

Alcaro commented 2 years ago

From a cursory glance, this looks like it'll just trade one bug for another, and leak freespace in cases like 

;patch1.asm
org $008000
autoclean jsl hijack1
autoclean jsl hijack2

freecode
hijack1:
rtl

freecode
hijack2:
rtl

;patch2.asm
org $008000
autoclean jsl hijack1
autoclean jsl hijack2

freecode
hijack1:
rtl
hijack2:
rtl

or, of course, things like autoclean dl or autoclean read3($00f606) (this patch only touches the autoclean jml/jsl handler).

If I remember correctly, autoclean originally ran in the first pass, and the freespace finder was second; can't delete something that's not there yet. Did we leave that model? If yes, was it due to bugs involving lda ForwardLabel : autoclean jsl AnotherLabel?

Proposed proper patch: For each autoclean, loop the list of freespaces created in this patch prior to this point. If that list contains this address, the autoclean target was already deleted and reused, and should not be deleted again.

RPGHacker commented 2 years ago

No test added. Since this bug requires existing RATS tags to repro, I don't think the current testing framework would support a test for this.

Actually, I think there is a way to tell the test suite to a apply a patch twice to the same ROM. We just probably forgot to document it in README.me.

Taking a peak at prot.asm, the syntax seems to be: ;`+ #2

spooonsss commented 2 years ago

Alcaro is right, that testcase shows a bug introduced. And I forgot to handle autoclean dl. I'll rework this.

Alcaro commented 2 years ago

Looks good to me, other than the nitpick. Good job wrangling this thing, Asar is a messy tool and this isn't its easiest part.

randomdude999 commented 9 months ago

i deleted this entire fix in asar2.0, since there rats tags are cleaned during pass 1 and freespaces are allocated at the end of pass 1. which i'm pretty sure avoids the root cause of this bug