JSR Returning to wrong place in code

I live! And  have resurrected my old project...  and have encountered a problem.

For some reason, my JSRs are returning to a different place  in the code than they started. Has anyone run across  this?  The places  it's returning to aren't even JSRs,  just random parts  in the code.  I'm not even sure where to  start fixing this.

It's usually a problem with the stack. This happens when you push inside a function but don't pull before the RTS/RTL. Same thing if you push then do a JSR then pull or another thing, pushing twice and only pulling once. But this is really common mistake for me and for a lot of ASM newbies, maybe you're talking about something else...

Edit: Or are you saying your JSR goes to another offset than the opcode params???

No, you're right that I was talking about the RTS. I wondered about pushing/pulling being an issue, but I'm not seeing any mismatches in the code anywhere.

I'll post the code snippet tomorrow, when I can get my laptop to some wifi. XD I'm sure I'm just missing something obvious because I've been staring at the code too long.

Man, I did a whole stream on this subject. But the I had to stop/start the stream a few times changing resolution cause my internet was dumb... otherwise I'd have it on youtube.

Basically, when you JSR it pushes your current location to the stack so that the RTS knows where to come back to. RTS takes the last 2 bytes on the stack (pushed from your JSR) as a return address then adds 1 to get to the next byte of code. This is why as a general rule, you don't f*** with the stack. however, if you know where you are in the stack its all usable.

Keep an eye out for maybe a push or pull inside a conditional that may be skipped due to a branch. That's the place I most often screw up the stack.

Okay, agh, I haven't done much with the stack in previous hacks (and also I haven't been doing much assembly for, uh... since whenever I was last around, at the very latest, so I'm out of practice). I did find a PLA nested inside a conditional that was being skipped! So I swapped that out with a LDA $01,S and moved the PLA to the end of the code patch, and... now it's RTSing to a DIFFERENT wrong position.

I can't win for losing.

Here's my code -- the start of the function is from vanilla, and my modified/new code starts at C2/5587.

Code:

C2/5551: 08           PHP
C2/5552: A2 1A        LDX #$1A
C2/5554: 9E BA 30     STZ $30BA,X    (set "position of spell" for every Esper to 0,
                                     because the 1-entry Esper menu is always at
                                     Position 0, just before the Magic menu.)
C2/5557: CA           DEX
C2/5558: 10 FA        BPL $5554      (iterate for all 27 Espers)
C2/555A: A9 FF        LDA #$FF
C2/555C: A2 35        LDX #$35
C2/555E: 9D A0 11     STA $11A0,X    (null out a temporary list)
C2/5561: CA           DEX
C2/5562: 10 FA        BPL $555E      (which has 54 entries)
C2/5564: A0 17        LDY #$17       (there are 24 possible known Lores)
C2/5566: A2 02        LDX #$02
C2/5568: 7B           TDC
C2/5569: 38           SEC
C2/556A: 6A           ROR
C2/556B: 90 02        BCC $556F      (have we looped a multiple of 8 times yet?)
C2/556D: 6A           ROR
C2/556E: CA           DEX            (if we're on our 9th/17th, set Bit 7 and repeat
                                     the process again.)
C2/556F: 3C 29 1D     BIT $1D29,X    (is current Lore known?)
C2/5572: F0 10        BEQ $5584      (branch if not)
C2/5574: EE 87 3A     INC $3A87      (increment number of known Lores)
C2/5577: 48           PHA
C2/5578: 98           TYA            (A = Lore ID)
C2/5579: 69 37        ADC #$37       (turn it into a position relative to Esper menu,
                                     which immediately precedes the Magic menu.)
C2/557B: 99 0F 31     STA $310F,Y    (save to "list of positions of each lore")
C2/557E: 69 54        ADC #$54       (so now we've converted our 0-23 Lore ID into a
                                     raw spell ID, as Condemned [8Bh] is the first
                                     Lore.)
C2/5580: 99 6A 30     STA $306A,Y    (save spell ID to "list of known Lores")
C2/5583: 68           PLA
C2/5584: 88           DEY
C2/5585: 10 E3        BPL $556A      (iterate 24 times, going through the Lores in
                                     reverse order)

Code:

!Terra = #$00    ; character IDs; I'm moving Celes and Relm up from their original positions
!Celes = #$01    ; to make handling magic stuff a little easier.
!Relm = #$02

org $C25587
LDA #$FF
LDX #$35
NullLists:
STA $3034,X
STA $11A0,X
DEX
BPL NullLists

LDY #$35           ; Y = Spell ID
BuildSpellList:
LDA $1A6E,Y        ; Spell known/not known
CMP $FF            ; Do we know this spell?
BEQ NextSpell      ; Branch if not
TYA                ; A = spell ID
STA $11A0,Y        ; set this entry in 'spells known' to spell ID
NextSpell:
DEY
BPL BuildSpellList

TDC
TAX
TAY
CondenseSpellListWhite:
LDA $11A0,X        ; Get Spell ID from temporary list
INC
BNE CondenseListWhite
LDA $11A1,X        ; Get other Spell ID from temporary list
INC
BEQ NextRowWhite
CondenseListWhite:
LDA $11A0,X
STA $3034,Y
LDA $11A1,X
STA $3035,Y
INY
INY
NextRowWhite:
INX
INX
CPX #$12
BCC CondenseSpellListWhite

TDC
TAX
TAY
CondenseSpellListBlack:
LDA $11B2,X        ; Get Spell ID from temporary list
INC
BNE CondenseListBlack
LDA $11B3,X        ; Get other Spell ID from temporary list
INC
BEQ NextRowBlack
CondenseListBlack:
LDA $11B2,X
STA $3046,Y
LDA $11B3,X
STA $3047,Y
INY
INY
NextRowBlack:
INX
INX
CPX #$12
BCC CondenseSpellListBlack   ; 99 bytes

TDC
TAX
TAY
CondenseSpellListGrey:
LDA $11C4,X        ; Get Spell ID from temporary list
INC
BNE CondenseListGrey
LDA $11C5,X        ; Get other Spell ID from temporary list
INC
BEQ NextRowGrey
CondenseListGrey:
LDA $11C4,X
STA $3058,Y
LDA $11C5,X
STA $3059,Y
INY
INY
NextRowGrey:
INX
INX
CPX #$12
BCC CondenseSpellListGrey

LDX #$35
BuildListPosition:
LDA $3034,X
CMP #$FF
BEQ PosiNextSpell

TAY          ; Y = spell ID
TXA          ; A = position in list
INC          ; Make it 1-based
CPY #$12     ; Is it White Magic?
BCC StorePosition ; If so, branch and store position
SBC #$12     ; Otherwise subtract 18d/12h from position for Black Magic menu
CPY #$24     ; Is it Black Magic?
BCC StorePosition ; If so, branch and store position
SBC #$12     ; Otherwise subtract from position for Grey Magic menu
StorePosition:
STA $3084,Y
PosiNextSpell:
DEX
BPL BuildListPosition

REP #$10     ; code originally starts at C2/5630
LDX #$004D   ; to cycle through all spells and lore/summons
BuildCharLists:
TDC
LDA $3034,X  ; get spell ID
CMP #$FF     ;
BEQ CharNextSpell ; branch to next slot if null
PHA          ; save spell ID
TAY          ;
LDA $3084,Y  ; get spell position relative to Esper menu
REP #$20
ASL
ASL          ; four bytes per spell entry
SEP #$20
TAY          ; Y = spell position relative to esper menu
LDA $01,S    ; retrieve spell ID from stack
CMP #$8B     ; are we at first lore/summon?
BCS IsSummon ; branch if it is
JSR NotSummon ; otherwise jump to build spell list for Terra/Relm/Celes
BRA CharNextSpell

IsSummon:
SBC #$8B     ; otherwise turn it into a 0-23 ID rather than raw spell ID
STA $208E,Y  ; store summon in first character's menu
STA $21CA,Y
STA $2306,Y
STA $2442,Y
LDA $01,S    ; I had this as a PLA (and the PLA below wasn't there), but realised it was being skipped
             ; if NotSummon was taken. I think. UGH.
JSR $5723    ; from spell data, put aiming byte in bottom half of A,
            ; and MP cost in top half
STA $2090,Y  ; store aiming byte in 1st character's menu
STA $21CC,Y
STA $2308,Y
STA $2444,Y
XBA          ; get MP cost
STA $2091,Y  ; store in first character's menu
STA $21CD,Y
STA $2309,Y
STA $2445,Y

CharNextSpell:
DEX
BPL BuildCharLists
PLA       ; This was just above before, but was being skipped if NotSummon was being jumped to.
PLP
RTS       ; 149 bytes (248 bytes down from 261)



org $C2163B            ; originally code for Umaro's attacks
NotSummon:   ; coming into here, A = spell ID, Y = spell position, X = list index
            ; the first item on the stack is also the spell ID
PHX          ; store our list index
LDX #$0006   ; and then load our character-checking index
CMP #$12     ; is spell white magic?
BCC IsCeles  ; if so, check if we have Celes in the party
CMP #$24     ; is it black magic?
BCC IsTerra  ; if so, check if we have Terra in the party
            ; otherwise, it's grey magic, so we check for Relm

IsRelm:
LDA $3ED8,X  ; get current character
CMP !Relm
BEQ AddToList ; x = now equals the party member who is Relm
DEX
DEX
BPL IsRelm
BRA GoToNextSpell          ; otherwise, Relm isn't in the party and we don't need to build her menu

IsCeles:
LDA $3ED8,X  ; get current character
CMP !Terra
BEQ AddToList
DEX
DEX
BPL IsCeles
BRA GoToNextSpell

IsTerra:
LDA $3ED8,X  ; get current character
CMP !Celes
BEQ AddToList
DEX
DEX
BPL IsTerra
BRA GoToNextSpell

AddToList:
TXA
XBA
LDA #$9E    ; our offset between spell lists is 13Ch; 9Eh is half, and because X is position*2, it balances out
JSR $4781
REP #$20
CLC
ADC #$208E
STA $F0     ; $F0 is now the starting address of character's in-battle spell list
SEP #$20
LDA $01,S   ; retrieve spell ID from the stack
STA ($F0),Y   ; store spell ID in current party member's spell list
JSR $5723   ; from spell data, put aiming byte in bottom half of A, and MP cost in top half
INY
INY
STA ($F0),Y   ; store aiming byte in current party member's spell list
XBA
INY
STA ($F0),Y   ; store MP cost in current party member's spell list
GoToNextSpell:
PLX         ; retrieve our list index
RTS

It's jumping correctly to NotSummon (actually 'Lore' ability, but I'm co-opting Lore for summoning, rather than using the existing Esper function, in my hack) and then back; it's the RTS at the end of CharNextSpell that's causing problems.

From what B-Run is saying, I suspect I'm missing something SOMEWHERE with the stack, but I'm not experienced enough with it to tell where. XD Am I misunderstanding how to use LDA $01,S? From the way it's used in the base code, it looks like it was reading the last value of A that had been pushed to the stack, without actually pulling it off, but I'm definitely open to being wrong about that.

From a first quick look you're doing a PHA for "save spell ID". The NotSummon branch is ok (one PHX, one PLX). However the IsSummon has zero push two pull (PLA, PLP) at the end and NotSummon eventually end there too to at CharNextSpell. What you do in fact is restoring the flags according to the stack value preceding your initial PHA. Is the PLP really neccesary? If you really need to restore the flags or clear them do something like the following:

This clear all flags

Code:

LDA #$00
PHA
PLP

Edit: Adding a PHP at beginning of your custom code should work too.

The PLP is actually in reference to a PHP at the very start of the original codeblock (C2/5551), before the part I started changing, and the PLA is from a previously PHA under BuildCharLists.

Some of this is derived from vanilla code -- they used PHA/LDA $01,S to keep track of the spell ID while leaving it on the stack, and then using PLA instead for the last time it was being used. Initially, I had a PLA under IsSummon where it's now a LDA $01,S, right before that JSR $5723, but realised that it was being skipped if it was branching after NotSummon.

I'm clearly doing SOMETHING wrong with the stack, but all of my PH*/PL* are paired properly, as far as I can tell.

One thing I need to look at, actually, is LDA $01,S in the middle of NotSummon, although it's (probably) not causing the immediate problem -- would that be pulling the right value from the stack? It seems to be, from watching the accumulator while the stepping the code, but if the JSR pushes the address to the stack, would that interfere? XD Clearly I have much to learn about how the stack works.

LDA $01,S in AddToList pulls the first byte from the stack ($0001FF). Since we come from JSR NotSummon I think the spell index is shifted in $0001FD and $0001FE-$0001FF is occupied by the address. I think the correct thing to do here would be LDA $03,S but I'm not 100% sure as I never use this type of LDA. However even if you load from the stack a LDA does not alter the stack like a push/pull does.

I'm not sure we are solving the root problem here... :/

Yeah, it's definitely a side issue, but it does indicate (along with a little further reading) that I had some fundamental misunderstandings about how the stack works. I'll go over my code again with that in mind and see if I can track down the problem now.

ETA: I figured it out! I need to pull both inside IsSummon AND right after I return from NotSummon, before I branch, rather than at CharNextSpell. I realised that the address it was RTSing to was actually the IDs for the two Lore/Summon spells that were learned at the point I was testing this, because it wasn't pulling the value I'd pushed from A during that loop.

Things aren't actually working yet, but I've fixed the immediate problem. I'm at the actual menu-building code part now and I've barely started to scratch the surface of that. WISH ME LUCK.

Things aren't actually working yet, but I've fixed the immediate problem. I'm at the actual menu-building code part now and I've barely started to scratch the surface of that. WISH ME LUCK.

I'm glad you fixed this! I've never done any serious battle menu coding but B-Run has a good understanding of this I think with his work on CoV.