m68k: fix a5 frame pointer register clobbering

codeGenIntermediate/src/prog8/codegen/intermediate/StackFrameLayout.kt

@@ -20,6 +20,8 @@ import prog8.intermediate.*
  *  - it contains no inline assembly chunk,
  *  - it makes no indirect calls and its address is never taken (subptr dispatch tables, interrupt
  *    handler registration, ...): an unknown caller cannot push arguments into a frame,
+ *  - it cannot reach a subroutine whose assembly code writes to A5: such a call would overwrite
+ *    the frame pointer while this subroutine's frame slots are still live,
  *  - all its parameters are frameable (a parameter that cannot move forces the whole
  *    subroutine to keep the static convention, since the caller must then write static cells),
  *  - every local it moves is plain data (byte/word/long/pointer scalar or numeric/bool array)
@@ -80,11 +82,13 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro
         val dispatchTargets = collectDispatchTargets(subLabels, addressTaken)
         val indirectCallers = allSubs.filter { sub -> containsIndirectCall(sub) }.map { it.label }.toSet()
         val reentrant = subsWithMultipleLiveActivations(callSites, indirectCallers, dispatchTargets)
+        val framePointerClobberers = framePointerClobberingSubs()
+        val reachesFramePointerClobber = subsReachingFramePointerClobber(framePointerClobberers)
 
         val framedSubs = mutableMapOf<String, FrameLayoutResult>()
         val notFramedReasons = mutableMapOf<String, String>()
         for (sub in allSubs) {
-            val outcome = frameableSubroutine(sub, referencedIn, addressTaken, asmTexts, callSites, dispatchTargets, indirectCallers, reentrant)
+            val outcome = frameableSubroutine(sub, referencedIn, addressTaken, asmTexts, callSites, dispatchTargets, indirectCallers, reentrant, reachesFramePointerClobber)
             when (outcome) {
                 is Frameability.Frameable -> framedSubs[sub.label] = outcome.layout
                 is Frameability.NotFrameable -> notFramedReasons[sub.label] = outcome.reason
@@ -148,6 +152,125 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro
         return found
     }
 
+    /**
+     * Labels of subroutines whose code writes to A5, the frame pointer register of a framed
+     * subroutine. Assembly subroutines are matched on their assembly text, ordinary subroutines on
+     * the text of their inline assembly chunks (a subroutine without assembly cannot write A5).
+     *
+     * Such a subroutine invalidates the frame of its caller: every frame-slot reference in the
+     * caller then uses a different base register, and the caller's `unlk a5` restores a frame
+     * pointer that was never saved.
+     */
+    private fun framePointerClobberingSubs(): Set<String> {
+        val result = mutableSetOf<String>()
+        program.allAsmSubs().forEach { asmSub ->
+            if (writesFramePointerRegister(asmSub.asmChunk.assembly))
+                result += asmSub.label
+        }
+        for (sub in program.allSubs()) {
+            sub.forEachChunk { chunk ->
+                if (chunk is IRInlineAsmChunk && writesFramePointerRegister(chunk.assembly))
+                    result += sub.label
+            }
+        }
+        return result
+    }
+
+    /**
+     * Subroutine label -> label of the frame-pointer-clobbering subroutine it can reach, for every
+     * subroutine that can (directly or indirectly) call one. An indirect call may dispatch to any
+     * subroutine, so a subroutine containing one is assumed to reach all of them.
+     */
+    private fun subsReachingFramePointerClobber(clobberers: Set<String>): Map<String, String> {
+        if (clobberers.isEmpty())
+            return emptyMap()
+        val callTargets = mutableMapOf<String, MutableSet<String>>()
+        for (sub in program.allSubs()) {
+            val targets = callTargets.getOrPut(sub.label) { mutableSetOf() }
+            sub.forEachChunk { chunk ->
+                chunk.instructions.forEach { instr ->
+                    when (val ref = instr.callSite?.codeReference) {
+                        is CodeReference.Label -> targets += ref.name
+                        is CodeReference.Indirect -> targets += clobberers
+                        else -> {}
+                    }
+                }
+            }
+        }
+        val reaching = mutableMapOf<String, String>()
+        var changed = true
+        while (changed) {
+            changed = false
+            for ((label, targets) in callTargets) {
+                if (label in reaching)
+                    continue
+                val hit = targets.firstOrNull { it in clobberers || it in reaching } ?: continue
+                reaching[label] = if (hit in clobberers) hit else reaching.getValue(hit)
+                changed = true
+            }
+        }
+        return reaching
+    }
+
+    /**
+     * True when the assembly text writes to A5 without protecting the caller's frame pointer, in a
+     * form this textual check recognises: as the destination operand of an instruction, as the frame
+     * register of a `link`, or anywhere in a `movem` register list (whose mask syntax is not parsed
+     * here). The usual `move.l a5,-(sp)` ... `move.l (sp)+,a5` prologue/epilogue pair is recognised
+     * as balanced and does not count; a write in between does.
+     *
+     * The scan is linear and ignores branches, so a subroutine that saves A5 on one path and
+     * overwrites it on another is reported conservatively as a clobber. Being over-cautious only
+     * costs the calling subroutine its stack frame (it keeps the static convention instead).
+     */
+    private fun writesFramePointerRegister(asm: String): Boolean {
+        var saved = false
+        for (rawLine in asm.lines()) {
+            // drop comments and a leading label, so a labelled instruction is still examined
+            val line = rawLine.substringBefore(';')
+                .replace(Regex("^[.0-9A-Za-z_$]+:"), "")
+                .trim().replace(Regex("[ \t]+"), " ")
+            if (line.isEmpty() || line.startsWith('.'))    // equates and directives
+                continue
+            val words = line.split(' ')
+            val mnemonic = words[0].lowercase()
+            val operands = words.drop(1).joinToString(",").split(',')
+            if (operands.any { it.isEmpty() })
+                continue
+            if (mnemonic.startsWith("movem")) {
+                val pushes = operands.last().lowercase() == "-(sp)"
+                val mentionsA5 = operands.any { it.lowercase().contains("a5") }
+                if (pushes && mentionsA5) {
+                    saved = true
+                    continue
+                }
+                if (!pushes && operands.first().lowercase() == "(sp)+" && mentionsA5) {
+                    saved = false
+                    continue
+                }
+                if (mentionsA5 && !saved)
+                    return true
+                continue
+            }
+            if (mnemonic == "link" && operands[0].lowercase().removePrefix("-") == "a5")
+                return true
+            val source = operands.first().lowercase()
+            val destination = operands.last().lowercase()
+            val writesA5 = destination == "a5" || destination == "-a5"
+            if (destination == "-(sp)" && source == "a5") {
+                saved = true
+                continue
+            }
+            if (source == "(sp)+" && writesA5 && operands.size == 2) {
+                saved = false
+                continue
+            }
+            if (writesA5 && !saved)
+                return true
+        }
+        return false
+    }
+
     /**
      * Labels of subroutines that can have two live activations at the same time: they lie on a
      * cycle of direct calls (direct recursion or mutual recursion), or on a cycle that only closes
@@ -256,10 +379,14 @@ class StackFrameLayout(private val program: IRProgram, private val errors: IErro
         callSites: Map<String, List<CallSiteRef>>,
         dispatchTargets: Set<String>,
         indirectCallers: Set<String>,
-        reentrant: Set<String>
+        reentrant: Set<String>,
+        reachesFramePointerClobber: Map<String, String>
     ): Frameability {
         val static = { reason: String -> Frameability.NotFrameable(reason) }
         if (sub.hasFrame) return static("it already has a stack frame")
+        reachesFramePointerClobber[sub.label]?.let {
+            return static("it calls '$it', whose assembly code overwrites the frame pointer register A5")
+        }
         var hasInlineAsm = false
         sub.forEachChunk { chunk -> if (chunk is IRInlineAsmChunk) hasInlineAsm = true }
         if (hasInlineAsm) return static("it contains inline assembly")

compiler/res/prog8lib/shared_amiga_custom.p8

@@ -24,6 +24,7 @@ IRQ5 = $74
 IRQ6 = $78
 IRQ7 = $7C
 
+            move.l  a5,-(sp)        ; a5 is the frame pointer register, but it is needed below for Supervisor()
             move.l  4.w,a6
             jsr     exec.Forbid(a6)        ; Do not run other tasks
             btst.b  #0,exec_AttnFlags+1(a6)    ; Check if > 68000 processor
@@ -65,7 +66,8 @@ IRQ7 = $7C
 .getvbr:    dc.l    $4E7A0801            ; MOVEC VBR,d0  - privileged instruction
             rte
 
-.skip:      rts
+.skip:      move.l  (sp)+,a5        ; restore the caller's frame pointer
+            rts
 
         }}
     }

compiler/res/prog8lib/shared_m68k_adpcm.p8

@@ -284,12 +284,14 @@ adpcm {
         decode_block_stereo_loop(nibblesptr, outptr)
     }
 
-    private asmsub decode_block_stereo_loop(pointer nibblesptr @A0, pointer outptr @A1) clobbers (D0, D1, D2, D3, D4, D5, D6, D7, A0, A1, A2, A3, A4, A5) {
+    private asmsub decode_block_stereo_loop(pointer nibblesptr @A0, pointer outptr @A1) clobbers (D0, D1, D2, D3, D4, D5, D6, D7, A0, A1, A2, A3, A4) {
         ; Decode 248 nibbles in 31 outer iterations, each processing 4 left bytes
         ; then 4 right bytes.  D7/D6 = left predict/(rowindex+1)*32, D5/D3 = right,
         ; D4 = loaded longword, D2 = temp word, A5 = outer loop counter.
         ; Left samples go to (a1)/4(a1) and right samples to 2(a1)/6(a1) via a4,
         ; producing interleaved L,R output directly with no separate interleave pass.
+        ; A5 is used as scratch here, but it is the caller's frame pointer register, so it is
+        ; saved and restored around the loop.
         %asm {{
         lea     p8b_adpcm.p8v_deltas_table-32,a2
         lea     p8b_adpcm.p8v_next_state_table,a3
@@ -303,6 +305,7 @@ adpcm {
         addq.w  #1,d6
         lsl.w   #5,d6          ; S_left = (rowindex + 1) * 32
         move.w  p8b_adpcm.p8v_predict,d7
+        move.l  a5,-(sp)        ; preserve the caller's frame pointer register
         suba.l  a5,a5
         move.w  #31,a5
 .loop:
@@ -508,6 +511,7 @@ adpcm {
         lsr.w   #5,d1
         subq.w  #1,d1
         move.b  d1,p8b_adpcm.p8v_rowindex_2
+        move.l  (sp)+,a5        ; restore the caller's frame pointer register
         rts
         }}
     }

compiler/test/codegeneration/TestM68k.kt

@@ -519,4 +519,125 @@ main {
         startIdx shouldBeGreaterThan -1
         (sizeIdx < startIdx) shouldBe true
     }
+
+    test("subroutine calling assembly that overwrites a5 keeps the static convention") {
+        // A5 is the frame pointer of a framed subroutine. An asmsub that clobbers it without
+        // preserving it would silently invalidate the caller's frame slots (and its `unlk a5`),
+        // so such a caller must not be given a stack frame at all.
+        val src = """
+main {
+    inline asmsub clobberframepointer() {
+        %asm {{
+            moveq #1, d0
+            lea.l some_label(PC),a5
+some_label:
+            rts
+        }}
+    }
+
+    sub start() {
+        ubyte localvar
+        clobberframepointer()
+        localvar = 1
+    }
+}
+"""
+        val result = compileText(Amiga500Target(), optimize = false, src, outputDir, writeAssembly = true, assemble = false)
+        result shouldNotBe null
+        val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText()
+        val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start")
+        val end = asm.indexOf("; End of subroutine: p8b_main.p8s_start")
+        val lines = asm.substring(start, end).lines().map { it.trim() }
+        lines.none { it.startsWith("link  a5") } shouldBe true
+        lines.none { it.startsWith("unlk") } shouldBe true
+        // the local keeps its static cell, so no frame-relative access at all
+        lines.any { it.contains("p8b_main.p8s_start.p8v_localvar") } shouldBe true
+    }
+
+    test("subroutine calling assembly that preserves a5 gets a stack frame") {
+        // the same program, but the asmsub saves and restores a5, so the frame stays valid
+        val src = """
+main {
+    inline asmsub preserveframepointer() {
+        %asm {{
+            move.l a5,-(sp)
+            moveq #1, d0
+            lea.l some_label(PC),a5
+some_label:
+            move.l (sp)+,a5
+            rts
+        }}
+    }
+
+    sub start() {
+        ubyte localvar
+        preserveframepointer()
+        localvar = 1
+    }
+}
+"""
+        val result = compileText(Amiga500Target(), optimize = false, src, outputDir, writeAssembly = true, assemble = false)
+        result shouldNotBe null
+        val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText()
+        val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start")
+        val end = asm.indexOf("; End of subroutine: p8b_main.p8s_start")
+        val lines = asm.substring(start, end).lines().map { it.trim() }
+        lines.count { it.startsWith("link  a5") } shouldBe 1
+        lines.count { it.startsWith("unlk") } shouldBe 1
+        lines.any { it.contains("-1(a5)") } shouldBe true
+    }
+
+    test("custom.grab_system preserves the frame pointer register a5") {
+        // grab_system needs a5 to pass the VBR-reading helper to exec.Supervisor(), and A5 is now
+        // the frame pointer of the calling subroutine, so it must be saved and restored.
+        val src = """
+%import custom
+
+main {
+    sub start() {
+        custom.grab_system()
+        custom.restore_system()
+    }
+}
+"""
+        val result = compileText(Amiga500Target(), optimize = false, src, outputDir, writeAssembly = true, assemble = false)
+        result shouldNotBe null
+        val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText()
+        val grab = asm.substring(asm.indexOf("custom.grab_system:"), asm.indexOf("; End of subroutine: custom.grab_system"))
+        grab.lines().map { it.substringBefore(';').trim() }.let { lines ->
+            lines.count { it == "move.l  a5,-(sp)" } shouldBe 1
+            lines.count { it == ".skip:      move.l  (sp)+,a5" } shouldBe 1
+        }
+    }
+
+    test("adpcm stereo block decoder preserves the frame pointer register a5") {
+        // the stereo loop kernel uses A5 as its outer loop counter, but A5 is now the frame pointer
+        // of every framed Prog8 subroutine, so it must save and restore it around the loop.
+        val src = """
+%import adpcm
+
+main {
+    pointer inbuf = memory("inbuf", 256, 0)
+    pointer outbuf = memory("outbuf", 1024, 0)
+
+    sub start() {
+        adpcm.decode_block_stereo(inbuf, outbuf)
+    }
+}
+"""
+        val result = compileText(Amiga500Target(), optimize = false, src, outputDir, writeAssembly = true, assemble = false)
+        result shouldNotBe null
+        val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText()
+        val kernel = asm.substring(
+            asm.indexOf("p8b_adpcm.p8s_decode_block_stereo_loop:"),
+            asm.indexOf("; End of subroutine: p8b_adpcm.p8s_decode_block_stereo_loop")
+        )
+        kernel.lines().map { it.substringBefore(';').trim() }.let { lines ->
+            lines.count { it == "move.l  a5,-(sp)" } shouldBe 1
+            lines.count { it == "move.l  (sp)+,a5" } shouldBe 1
+        }
+        // its Prog8 caller and everything above it keep their stack frames
+        asm.contains("; ---- Subroutine: p8b_adpcm.p8s_decode_block_stereo ----") shouldBe true
+        asm.contains("link  a5,#") shouldBe true
+    }
 })

docs/source/technical.rst

@@ -280,6 +280,11 @@ assembly that calls a normal Prog8 subroutine must follow the stack convention a
    ``on..goto`` and ``on..call`` jump tables remain static because their symbolic initializers
    cannot currently be placed in a frame.
 
+   Assembly code that writes to ``A5`` without preserving the caller's frame pointer would
+   invalidate every frame slot of its caller, so a subroutine that (directly or indirectly) calls
+   such assembly is not given a stack frame and keeps the static convention instead. The usual
+   ``move.l a5,-(sp)`` ... ``move.l (sp)+,a5`` prologue and epilogue is recognised as balanced.
+
    A single local frame is limited to 16 KiB. The compiler reports oversized frames, but does
    not detect exhaustion caused by a deep call chain or recursion. Frame-slot reuse is also
    conservative in routines containing loops, so such routines may use more stack space than

examples/amiga/lowlevel-timer.p8

@@ -139,4 +139,4 @@ main {
         txt.print("clean exit\n")
         sys.exit(0)
     }
-}
+}