mirror of
https://github.com/irmen/prog8.git
synced 2026-10-22 05:02:58 +00:00
fix Status flag extraction clobbers a register return value
This commit is contained in:
Irmen de Jong committed
1 parent
976b64185d
commit
4cbf93da85
8 files changed
+232
-293
No files matched your search
@@ -68,10 +68,15 @@ internal class AssignmentAsmGen(
|
||||
|
||||
// Check if we need to save A register before extracting status flags
|
||||
// (flag extraction functions like assignCarryFlagResult overwrite A)
|
||||
val hasByteInA = registersResults.any { (ret, _) ->
|
||||
// A holds the low byte of a byte/bool return in @A, and of a word return in @AX/@AY
|
||||
val byteInA = registersResults.any { (ret, _) ->
|
||||
ret.type.isByteOrBool && ret.register.registerOrPair == RegisterOrPair.A
|
||||
}
|
||||
if(hasByteInA && statusFlagResults.isNotEmpty()) {
|
||||
val wordLowByteInA = registersResults.any { (ret, _) ->
|
||||
ret.type.isWord && ret.register.registerOrPair in setOf(RegisterOrPair.AX, RegisterOrPair.AY)
|
||||
}
|
||||
val hasAInResult = byteInA || wordLowByteInA
|
||||
if(hasAInResult && statusFlagResults.isNotEmpty()) {
|
||||
asmgen.out(" pha") // Save A (contains return value) before flag extraction
|
||||
}
|
||||
|
||||
@@ -101,7 +106,7 @@ internal class AssignmentAsmGen(
|
||||
}
|
||||
|
||||
// Restore A if we saved it for flag extraction
|
||||
if(hasByteInA && statusFlagResults.isNotEmpty()) {
|
||||
if(hasAInResult && statusFlagResults.isNotEmpty()) {
|
||||
asmgen.out(" pla")
|
||||
}
|
||||
|
||||
|
||||
@@ -33,9 +33,10 @@ internal class AssignmentGen(private val codeGen: IRCodeGen, private val exprGen
|
||||
if (extsub.returns.size == assignmentTargets.size) {
|
||||
// Targets and values match. Assign all the things. Skip 'void' targets.
|
||||
// We need to handle both FP registers (FAC1/FAC2) and regular CPU registers
|
||||
// Status flag returns MUST be processed first, because their branch-based
|
||||
// code needs to read CPU flags immediately after the call, before any
|
||||
// LOADHR/STOREM instructions clobber them.
|
||||
// Status flag returns must be read before anything that clobbers the CPU status
|
||||
// bits. When the call also has non-flag register returns, those are processed
|
||||
// first (they may clobber the scratch register used by the flag extraction),
|
||||
// protected by an outer PUSHST/POPST so the flags stay intact for the extraction.
|
||||
val fpRegs = funcCall.multipleResultFpRegs.toMutableList()
|
||||
val cpuRegs = funcCall.multipleResultRegs.toMutableList()
|
||||
|
||||
@@ -67,16 +68,32 @@ internal class AssignmentGen(private val codeGen: IRCodeGen, private val exprGen
|
||||
// flag still needs to read. Wrap every flag pair's handling in
|
||||
// PUSHST/POPST so each one sees the original status right after the call.
|
||||
val wrapStatusFlags = flagPairs.count { !isVoidTarget(it) } > 1
|
||||
flagPairs.forEach { pair ->
|
||||
if (wrapStatusFlags && !isVoidTarget(pair)) {
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) }
|
||||
processPair(pair)
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) }
|
||||
} else {
|
||||
processPair(pair)
|
||||
fun processFlagPairs() {
|
||||
flagPairs.forEach { pair ->
|
||||
if (wrapStatusFlags && !isVoidTarget(pair)) {
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) }
|
||||
processPair(pair)
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) }
|
||||
} else {
|
||||
processPair(pair)
|
||||
}
|
||||
}
|
||||
}
|
||||
otherPairs.forEach(::processPair)
|
||||
|
||||
val hasStatusFlags = flagPairs.any { !isVoidTarget(it) }
|
||||
val hasOtherResults = otherPairs.any { !isVoidTarget(it) }
|
||||
if (hasStatusFlags && hasOtherResults) {
|
||||
// The non-flag register returns are read before the flag extraction,
|
||||
// so the extraction's scratch register cannot clobber them. The flag
|
||||
// bits are preserved across the register loads/stores with PUSHST/POPST.
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) }
|
||||
otherPairs.forEach(::processPair)
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) }
|
||||
processFlagPairs()
|
||||
} else {
|
||||
processFlagPairs()
|
||||
otherPairs.forEach(::processPair)
|
||||
}
|
||||
} else {
|
||||
throw AssemblyError("number of values and targets don't match")
|
||||
}
|
||||
|
||||
@@ -180,21 +180,18 @@ internal fun AsmGen.translateControl(insn: IRInstruction, forwardedImmediateCall
|
||||
// === Status flag stack ops via Scc ===
|
||||
|
||||
Opcode.PUSHST -> {
|
||||
// Move CCR to D0 (low byte), then push as byte.
|
||||
// Save CCR to the stack. The 68010+ memory form keeps the CCR save
|
||||
// register-neutral (no data register is clobbered) and word-aligned.
|
||||
if(program.options.compTarget.cpu == CpuType.M68000)
|
||||
error("the 68000 cpu cannot save/restore the status bits using a nonprivileged instruction. This is required to implement the 'PUSHST/POPST' IR opcodes. Compile for 68010 or higher cpu or change the code such that PUSHST/POPST are no longer used (sometimes complicated rol/ror operations need it)")
|
||||
invalidateD0Cache()
|
||||
emitLine("move ccr, d0")
|
||||
emitLine("move.b d0, -(sp)")
|
||||
emitLine("move ccr, -(sp)")
|
||||
}
|
||||
|
||||
Opcode.POPST -> {
|
||||
// Pop byte into D0, then restore CCR
|
||||
// Restore CCR from the stack (register-neutral on 68010+).
|
||||
if(program.options.compTarget.cpu == CpuType.M68000)
|
||||
error("the 68000 cpu cannot save/restore the status bits using a nonprivileged instruction. This is required to implement the 'PUSHST/POPST' IR opcodes. Compile for 68010 or higher cpu or change the code such that PUSHST/POPST are no longer used (sometimes complicated rol/ror operations need it)")
|
||||
invalidateD0Cache()
|
||||
emitLine("move.b (sp)+, d0")
|
||||
emitLine("move d0, ccr")
|
||||
emitLine("move (sp)+, ccr")
|
||||
}
|
||||
|
||||
// === Flag manipulation ===
|
||||
|
||||
@@ -450,6 +450,52 @@ main {
|
||||
}
|
||||
}
|
||||
|
||||
test("status flag return is saved and register return read first on m68k") {
|
||||
// Regression test: a multi-assign returning both a status flag and a register value
|
||||
// used to let the flag extraction (which uses D0 as scratch for ROXL) clobber the
|
||||
// register return. The register return is now read first, protected by a
|
||||
// register-neutral PUSHST/POPST (the 68010+ 'move ccr,-(sp)' form).
|
||||
val src = """
|
||||
main {
|
||||
inline asmsub f(ubyte arg @D0) -> bool @Pc, ubyte @D0 {
|
||||
%asm {{
|
||||
moveq #42, d0
|
||||
cmp.b #42, d0
|
||||
}}
|
||||
}
|
||||
|
||||
sub start() {
|
||||
bool c
|
||||
ubyte v
|
||||
c, v = f(42)
|
||||
}
|
||||
}
|
||||
"""
|
||||
val result = compileText(Qemu68kTarget(), optimize = false, src, outputDir, writeAssembly = true, assemble = false)
|
||||
result shouldNotBe null
|
||||
val asm = result!!.compilationOptions.outputDir.resolve("${result.compilerAst.name}.asm").toFile().readText()
|
||||
// scope to the start subroutine region
|
||||
val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start")
|
||||
val end = asm.indexOf("; ---- Subroutine:", start + 1).let { if (it < 0) asm.length else it }
|
||||
val lines = asm.substring(start, end).lines().map { it.trim() }
|
||||
// register-neutral PUSHST/POPST form, no d0 round-trip
|
||||
lines.count { it == "move ccr,-(sp)" } shouldBe 1
|
||||
lines.count { it == "move (sp)+,ccr" } shouldBe 1
|
||||
lines.count { it == "move ccr, d0" } shouldBe 0
|
||||
lines.count { it == "move.b d0, -(sp)" } shouldBe 0
|
||||
// the @D0 return is read (loadhr) before the flag extraction's scratch use (roxl)
|
||||
val loadhrIdx = lines.indexOfFirst { it.startsWith("; loadhr.b") }
|
||||
val roxlIdx = lines.indexOfFirst { it.contains("roxl") }
|
||||
loadhrIdx shouldBeGreaterThan -1
|
||||
roxlIdx shouldBeGreaterThan -1
|
||||
(loadhrIdx < roxlIdx) shouldBe true
|
||||
// the loadhr sits inside the PUSHST/POPST sandwich
|
||||
val pushIdx = lines.indexOfFirst { it == "move ccr,-(sp)" }
|
||||
val popIdx = lines.indexOfFirst { it == "move (sp)+,ccr" }
|
||||
(pushIdx < loadhrIdx) shouldBe true
|
||||
(loadhrIdx < popIdx) shouldBe true
|
||||
}
|
||||
|
||||
test("struct type symbols are emitted in the header before program start") {
|
||||
val src = """
|
||||
main {
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package prog8tests.codegeneration
|
||||
|
||||
import io.kotest.core.spec.style.FunSpec
|
||||
import io.kotest.matchers.ints.shouldBeGreaterThan
|
||||
import io.kotest.matchers.shouldBe
|
||||
import prog8.code.target.C64Target
|
||||
import prog8tests.helpers.compileText
|
||||
@@ -187,4 +188,107 @@ class TestNewCodegenBugs : FunSpec({
|
||||
legacyMachine.assertMemory(0x343, expected[3])
|
||||
}
|
||||
}
|
||||
|
||||
test("status flag return does not clobber an A register return value") {
|
||||
// Regression test: in a multi-assign returning both a status flag and a byte in A,
|
||||
// the flag extraction (which uses A as scratch) used to destroy the byte return.
|
||||
// The register return is now read first, protected by a PUSHST/POPST (php/plp on 6502).
|
||||
val src = $$"""
|
||||
%option no_sysinit
|
||||
%launcher none
|
||||
%address $1000
|
||||
main {
|
||||
inline asmsub f(ubyte arg @A) -> bool @Pc, ubyte @A {
|
||||
%asm {{
|
||||
lda #42
|
||||
cmp #42
|
||||
}}
|
||||
}
|
||||
|
||||
sub start() {
|
||||
bool c
|
||||
ubyte v
|
||||
c, v = f(42)
|
||||
main.v = v
|
||||
main.c = c as ubyte
|
||||
}
|
||||
ubyte @shared v
|
||||
ubyte @shared c
|
||||
}
|
||||
""".trimIndent()
|
||||
// asm-text only, so this test does not require an external assembler
|
||||
val compileResult = compileText(C64Target(), false, src, outputDir, newCodegen = true, writeAssembly = true, assemble = false)!!
|
||||
val asm = compileResult.compilationOptions.outputDir.resolve(compileResult.compilerAst.name + ".asm").readText()
|
||||
// scope to the start subroutine region
|
||||
val start = asm.indexOf("; ---- Subroutine: p8b_main.p8s_start")
|
||||
val end = asm.indexOf("; ---- Subroutine:", start + 1).let { if (it < 0) asm.length else it }
|
||||
val lines = asm.substring(start, end).lines().map { it.trim() }
|
||||
// the register read (loadhr) must happen before the flag extraction's lda #0
|
||||
val loadhrIdx = lines.indexOfFirst { it.startsWith("; loadhr.b") }
|
||||
val flagLdaIdx = lines.indexOfFirst { it == "lda #0" }
|
||||
loadhrIdx shouldBeGreaterThan -1
|
||||
flagLdaIdx shouldBeGreaterThan -1
|
||||
(loadhrIdx < flagLdaIdx) shouldBe true
|
||||
// protected by a php/plp pair around the register read
|
||||
lines.count { it == "php" } shouldBe 1
|
||||
lines.count { it == "plp" } shouldBe 1
|
||||
}
|
||||
|
||||
test("legacy 6502 word @AX/@AY return saves A across status flag extraction") {
|
||||
val src = $$"""
|
||||
%option no_sysinit
|
||||
main {
|
||||
asmsub fAX(ubyte a @A) -> bool @Pc, uword @AX {
|
||||
%asm {{
|
||||
lda #42
|
||||
ldx #1
|
||||
cmp #42
|
||||
rts
|
||||
}}
|
||||
}
|
||||
asmsub fAY(ubyte a @A) -> bool @Pc, uword @AY {
|
||||
%asm {{
|
||||
lda #42
|
||||
ldy #1
|
||||
cmp #42
|
||||
rts
|
||||
}}
|
||||
}
|
||||
sub start() {
|
||||
bool c
|
||||
uword w
|
||||
c, w = fAX(1)
|
||||
main.w = w
|
||||
c, w = fAY(1)
|
||||
main.w = w
|
||||
}
|
||||
uword @shared w
|
||||
}
|
||||
""".trimIndent()
|
||||
// asm-text only, so this test does not require an external assembler
|
||||
val compileResult = compileText(C64Target(), false, src, outputDir, newCodegen = false, writeAssembly = true, assemble = false)!!
|
||||
val asm = compileResult.compilationOptions.outputDir.resolve(compileResult.compilerAst.name + ".asm").readText()
|
||||
// scope to the start subroutine region (legacy asm delimits top-level subroutines with a "<name>\t.proc" label at column 0)
|
||||
val rawLines = asm.lines()
|
||||
val from = rawLines.indexOfFirst { it.startsWith("p8s_start\t.proc") }
|
||||
val to = rawLines.drop(from + 1).indexOfFirst { it.endsWith(".proc") && !it.startsWith(" ") && !it.startsWith("\t") }
|
||||
.let { if (it < 0) rawLines.size else from + 1 + it }
|
||||
val lines = rawLines.subList(from, to).map { it.trim() }
|
||||
// the flag extraction clobbers A, which holds the low byte of the word result,
|
||||
// so A must be saved with pha and restored with pla before the pair is stored
|
||||
lines.count { it == "pha" } shouldBe 2
|
||||
lines.count { it == "pla" } shouldBe 2
|
||||
val phaIdx = lines.indexOfFirst { it == "pha" }
|
||||
val flagIdx = lines.indexOfFirst { it == "lda #0" }
|
||||
val plaIdx = lines.indexOfFirst { it == "pla" }
|
||||
// the first store of the word variable's low byte
|
||||
val staIdx = lines.indexOfFirst { it.startsWith("sta ") && it.endsWith("p8v_w") }
|
||||
phaIdx shouldBeGreaterThan -1
|
||||
flagIdx shouldBeGreaterThan -1
|
||||
plaIdx shouldBeGreaterThan -1
|
||||
staIdx shouldBeGreaterThan -1
|
||||
(phaIdx < flagIdx) shouldBe true
|
||||
(flagIdx < plaIdx) shouldBe true
|
||||
(plaIdx < staIdx) shouldBe true
|
||||
}
|
||||
})
|
||||
@@ -1753,6 +1753,43 @@ main {
|
||||
irContent shouldContain "bstvc"
|
||||
}
|
||||
|
||||
test("asmsub multi-assign with a status flag and a register return orders the register read first") {
|
||||
// The register return must be read before the flag extraction (which uses a
|
||||
// scratch hardware register), protected by an outer PUSHST/POPST.
|
||||
val src = """
|
||||
%option no_sysinit
|
||||
main {
|
||||
sub start() {
|
||||
ubyte v
|
||||
bool c
|
||||
v, c = test()
|
||||
main.r1 = v
|
||||
main.b1 = c
|
||||
}
|
||||
asmsub test() -> ubyte @A, bool @Pc {
|
||||
%asm {{
|
||||
lda #42
|
||||
cmp #42
|
||||
rts
|
||||
}}
|
||||
}
|
||||
ubyte @shared r1
|
||||
bool @shared b1
|
||||
}"""
|
||||
val result = compileText(VMTarget(), optimize=false, src, outputDir, writeAssembly = true)!!
|
||||
val virtfile = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".p8ir")
|
||||
val lines = virtfile.readText().lines().map { it.trim() }
|
||||
// one outer PUSHST/POPST pair (only one flag, so no per-flag wrapping)
|
||||
lines.count { it == "pushst" } shouldBe 1
|
||||
lines.count { it == "popst" } shouldBe 1
|
||||
// the register read (loadhr) must come before the flag extraction (roxl)
|
||||
val loadhrIdx = lines.indexOfFirst { it.startsWith("loadhr") }
|
||||
val roxlIdx = lines.indexOfFirst { it.startsWith("roxl") }
|
||||
loadhrIdx shouldBeGreaterThan -1
|
||||
roxlIdx shouldBeGreaterThan -1
|
||||
(loadhrIdx < roxlIdx) shouldBe true
|
||||
}
|
||||
|
||||
test("asmsub multi-assign with multiple status flags in one call saves the status around each flag") {
|
||||
val src = """
|
||||
%option no_sysinit
|
||||
@@ -1783,10 +1820,11 @@ main {
|
||||
val result = compileText(VMTarget(), optimize=false, src, outputDir, writeAssembly = true)!!
|
||||
val virtfile = result.compilationOptions.outputDir.resolve(result.compilerAst.name + ".p8ir")
|
||||
val irContent = virtfile.readText()
|
||||
// each of the three status flags needs its own PUSHST/POPST pair, because the
|
||||
// 1 outer PUSHST/POPST pair protects the register return read plus the flags across it,
|
||||
// and each of the three status flags needs its own PUSHST/POPST pair, because the
|
||||
// branch-based extraction of one flag clobbers the status for the following flags
|
||||
irContent.lines().count { it.trim() == "pushst" } shouldBe 3
|
||||
irContent.lines().count { it.trim() == "popst" } shouldBe 3
|
||||
irContent.lines().count { it.trim() == "pushst" } shouldBe 4
|
||||
irContent.lines().count { it.trim() == "popst" } shouldBe 4
|
||||
}
|
||||
|
||||
test("union zero-init via memory, write byte and read overlapping word") {
|
||||
|
||||
@@ -14,7 +14,6 @@ Future Things and Ideas
|
||||
- support typed pointer arrays as struct fields, curretly requires untyped pointers arrays.
|
||||
- allow aliasing a whole enum (``alias myEnum = MyEnum``) so that ``myEnum::Member`` works just like ``MyEnum::Member``; currently only aliasing individual enum members is supported. See ideas/alias-enums.md for the plan.
|
||||
- evaluate the feature suggestions in ideas/c16-banked-data.md (cx16 banked data library).
|
||||
- Status flag extraction clobbers a register return value: in a multi-assign where one call returns both a status flag and a regular register value (e.g. ``-> bool @Pc, ubyte @A`` on new6502, or ``-> bool @Pc, ubyte @D0`` on m68k), the status flag extraction destroys the register return before it is read. The extraction uses a scratch hardware register (``A`` on 6502, ``D0`` on m68k) that can be the same register holding the return value. The old 6502 codegen avoids this with ``pha``/``pla``. See ideas/status-flag-register-clobber.md for the design (reorder the register returns before the flag extraction, protected by register-neutral ``PUSHST``/``POPST``).
|
||||
|
||||
Won't do's or deferred
|
||||
^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
||||
@@ -1,267 +0,0 @@
|
||||
# Status flag extraction clobbers a register return value
|
||||
|
||||
**Status: deferred / design only, not implemented.** Listed in
|
||||
`docs/source/todo.rst`.
|
||||
|
||||
Note: the *multiple* status flag returns case is already fixed (each flag
|
||||
extraction is wrapped in `PUSHST`/`POPST` in `AssignmentGen`). This document
|
||||
covers the separate case where a status flag return is combined with a regular
|
||||
register return that lives in the extraction's scratch register.
|
||||
|
||||
## 1. Problem
|
||||
|
||||
In a multi-assignment where one call returns both a CPU status flag and a
|
||||
regular register value, the flag extraction can destroy the register return
|
||||
before it is read. Example:
|
||||
|
||||
```prog8
|
||||
main {
|
||||
inline asmsub f(ubyte arg @A) -> bool @Pc, ubyte @A {
|
||||
%asm {{
|
||||
lda #42
|
||||
cmp #42
|
||||
}}
|
||||
}
|
||||
|
||||
sub start() {
|
||||
bool c
|
||||
ubyte v
|
||||
c, v = f(42) ; v is NOT 42, it is the clobbered scratch value
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
This is reachable in practice, e.g. the f256 syslib `GETIN() -> bool @Pc, ubyte @A`
|
||||
(`examples/customtarget/libraries/f256/syslib.p8:39`).
|
||||
|
||||
The bug is *separate* from the "multiple status flag returns clobber each other"
|
||||
issue (which was fixed by wrapping each flag extraction in `PUSHST`/`POPST` in
|
||||
`AssignmentGen`). That fix protects the status bits; it does not protect a
|
||||
hardware register that holds a non-flag return value.
|
||||
|
||||
### When it triggers
|
||||
|
||||
The bug needs a **multi-assign** from an `asmsub`/`extsub` (only those can
|
||||
declare status flag returns) whose returned **status flag target is not `void`**
|
||||
and whose **register return target is not `void`**, where the register value
|
||||
lives in the extraction's scratch register:
|
||||
|
||||
- **new6502 / 6502**: scratch is `A`.
|
||||
- `@A` (byte/bool) return: broken with *any* flag (`@Pc`/`@Pz`/`@Pn`/`@Pv`),
|
||||
because every flag extraction loads a `0`/`1` immediate through `A`.
|
||||
- `@AX`/`@AY` (word) return: broken too, since `A` is its low byte.
|
||||
- `@X`/`@Y`-only returns: safe.
|
||||
- **m68k**: scratch is `D0`, but only the `@Pc` extraction uses it (the `ROXL`
|
||||
round-trip). `@Pz`/`@Pn`/`@Pv` lower to a branch plus a direct memory store.
|
||||
- `@D0` + `@Pc`: broken.
|
||||
- `@D0` + `@Pz`/`@Pn`/`@Pv`: safe.
|
||||
- `@D1` and higher: safe.
|
||||
- **virtual (VM)**: never (extraction only uses IR virtual registers).
|
||||
|
||||
The return order does not matter (flags are always extracted before the register
|
||||
returns), and it is irrelevant where the value is stored (variable, register, or
|
||||
memory).
|
||||
|
||||
## 2. Root cause
|
||||
|
||||
The IR generator (`codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt`)
|
||||
processes a multi-assign in this order:
|
||||
|
||||
1. **status flag returns first** (they must read the live CPU status right after
|
||||
the call), then
|
||||
2. **non-flag register returns** via `LOADHR` (hardware register -> IR virtual
|
||||
register).
|
||||
|
||||
The status flag extraction sequence uses a **scratch hardware register**:
|
||||
|
||||
| target | extraction scratch | where |
|
||||
| --- | --- | --- |
|
||||
| new6502 | `A` | `lda #0` before `ROXL`, and the `lda`-based stores |
|
||||
| m68k | `D0` | `ROXL` is lowered as `move.b regfile,d0; roxl.b #1,d0; move.b d0,regfile` (`codeGenM68k/.../InstrBitwise.kt:357`) |
|
||||
| virtual (VM) | none (operates on IR virtual registers only) | unaffected |
|
||||
|
||||
If a non-flag return lives in that same scratch register, the extraction
|
||||
destroys it before `LOADHR` reads it.
|
||||
|
||||
### Evidence (new6502, `-target c64 -newcodegen`)
|
||||
|
||||
```
|
||||
; call p8b_main.p8s_f():r1.b@Pc,r2.b@s0
|
||||
; inlined: p8b_main.p8s_f
|
||||
lda #42
|
||||
cmp #42
|
||||
; end inlined
|
||||
; load.b r1.b,#0.b
|
||||
lda #0 <-- clobbers A
|
||||
sta p8_regfile+0
|
||||
; roxl.b r1.b
|
||||
rol p8_regfile+0
|
||||
; storem.b r1.b,[c]
|
||||
lda p8_regfile+0
|
||||
sta c
|
||||
; loadhr.b r2.b,s0.b <-- reads the clobbered A, not 42
|
||||
sta p8_regfile+1
|
||||
```
|
||||
|
||||
### Evidence (m68k, `-target qemu68k`)
|
||||
|
||||
```
|
||||
bsr p8b_main.p8s_f
|
||||
; load.b r1.b,#0.b
|
||||
clr.b p8_regfile+0
|
||||
; roxl.b r1.b
|
||||
move.b p8_regfile+0,d0 <-- clobbers d0 (which held the @D0 return)
|
||||
roxl.b #1,d0
|
||||
; storem.b r1.b,[c]
|
||||
move.b d0,p8v_c
|
||||
; loadhr.b r2.b,s10.b
|
||||
; storem.b r2.b,[v]
|
||||
move.b d0,p8v_v <-- stores the clobbered d0
|
||||
```
|
||||
|
||||
### Legacy 6502 reference
|
||||
|
||||
The old 6502 codegen handles the byte/bool-in-`A` case with `pha`/`pla`:
|
||||
|
||||
- `codeGenCpu6502/.../assignment/AssignmentAsmGen.kt:69-76` - if a byte/bool
|
||||
return is in `A` and there are status flags, emit `pha` before the flag
|
||||
extraction.
|
||||
- `:103-106` - `pla` before storing the register results.
|
||||
|
||||
Note that `hasByteInA` (`:69`) only checks `ret.type.isByteOrBool &&
|
||||
registerOrPair == A`, so legacy does **not** cover a word return in `@AX`/`@AY`
|
||||
and shares that variant of the bug (the low byte in `A` is clobbered). The
|
||||
proposed design below covers all of these cases.
|
||||
|
||||
## 3. Design
|
||||
|
||||
### 3.1 Key enabler: register-neutral PUSHST/POPST on m68k
|
||||
|
||||
The recommended fix relies on `PUSHST`/`POPST` not clobbering any data register.
|
||||
On new6502 they are already neutral (`php`/`plp`). On m68k the current
|
||||
implementation goes through `d0`:
|
||||
|
||||
```
|
||||
move ccr,d0
|
||||
move.b d0,-(sp) ; PUSHST
|
||||
move.b (sp)+,d0
|
||||
move d0,ccr ; POPST
|
||||
```
|
||||
|
||||
The 68010 `MOVE CCR` instruction also supports a **memory** operand, so the
|
||||
round-trip is unnecessary:
|
||||
|
||||
```
|
||||
move ccr,-(sp) ; PUSHST (encoding 42E7)
|
||||
move (sp)+,ccr ; POPST (encoding 44DF)
|
||||
```
|
||||
|
||||
This saves/restores the CCR without touching any data register, and keeps the
|
||||
stack word-aligned (the current byte push does not, which the m68k coding
|
||||
conventions warn against).
|
||||
|
||||
Verified by assembling with `vasmm68k_mot -m68020` and by executing a CCR
|
||||
round-trip under `qemu-system-m68k -M virt -cpu m68020` (a program that sets
|
||||
C=1, pushes the CCR, clears the flags, pops the CCR back, then takes `if_cs`
|
||||
prints the expected result).
|
||||
|
||||
### 3.2 IR generator change (`AssignmentGen.translate`)
|
||||
|
||||
Only when a call has **at least one status-flag return AND at least one
|
||||
non-flag result** (register or float), reorder so the register/float returns are
|
||||
materialized first, protected by a single `PUSHST`/`POPST`:
|
||||
|
||||
```
|
||||
CALL
|
||||
PUSHST ; save post-call status S0
|
||||
[otherPairs: LOADHR + store] ; read return registers before extraction;
|
||||
; may clobber flags and scratch registers
|
||||
POPST ; restore S0
|
||||
[flagPairs: per-flag PUSHST/extract+store/POPST when >1 flag]
|
||||
```
|
||||
|
||||
- Register/float values are safely in IR virtual registers before the flag
|
||||
extraction clobbers the scratch register.
|
||||
- The float case (`LOADHFACZERO`/`LOADHFACONE`, which lowers to a `jsr` on 6502)
|
||||
is covered by the same outer save/restore.
|
||||
- The existing per-flag `PUSHST`/`POPST` sandwich (for >1 status flag) is kept
|
||||
unchanged; this change is orthogonal.
|
||||
- Flags-only and registers-only multi-assigns keep the current codegen (no
|
||||
extra instructions).
|
||||
|
||||
Pseudo-code in the extsub branch:
|
||||
|
||||
```kotlin
|
||||
val hasStatusFlags = flagPairs.any { !isVoidTarget(it) }
|
||||
val hasOtherResults = otherPairs.any { !isVoidTarget(it) }
|
||||
if (hasStatusFlags && hasOtherResults) {
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.PUSHST) }
|
||||
otherPairs.forEach(::processPair)
|
||||
result += IRCodeChunk(null, null).also { it += IRInstructions.simple(Opcode.POPST) }
|
||||
flagPairs.forEach { pair -> wrap-if-multi(pair) }
|
||||
} else {
|
||||
flagPairs.forEach { pair -> wrap-if-multi(pair) }
|
||||
otherPairs.forEach(::processPair)
|
||||
}
|
||||
```
|
||||
|
||||
The "status flags MUST be processed first" comment in `AssignmentGen.kt:36` is
|
||||
then no longer accurate; it becomes "status flags must be read before anything
|
||||
that clobbers them, which is guaranteed by the surrounding PUSHST/POPST".
|
||||
|
||||
### 3.3 m68k backend change (`codeGenM68k/.../InstrControl.kt:180-200`)
|
||||
|
||||
Change `PUSHST`/`POPST` to the memory form and drop the `d0` round-trip (and the
|
||||
now-unnecessary `invalidateD0Cache()` calls). The 68000 guard stays: the 68000
|
||||
cannot save the CCR with a nonprivileged instruction.
|
||||
|
||||
No changes are needed in new6502, the VM, or the IR file format.
|
||||
|
||||
## 4. Alternatives considered
|
||||
|
||||
- **New IR opcodes `PUSHHR`/`POPHR`** (save/restore a hardware register slot)
|
||||
around the flag extraction. Works, and keeps `PUSHST`/`POPST` semantics
|
||||
unchanged, but requires IR enum changes, `IRTextCodec`/reader support,
|
||||
peephole side-effect sets, VM support, and implementations in both backends.
|
||||
More plumbing for the same result.
|
||||
- **d0-preserving `PUSHST`** (push `d0` plus the CCR, restore both). Legal on
|
||||
all m68k CPUs, but leaves `d0` clobbered *between* `PUSHST` and `POPST`, so a
|
||||
`LOADHR d0` inside the wrap still reads garbage. Rejected.
|
||||
- **6502-only reorder without save/restore.** On 6502 `LOADHR` (`sta`/`stx`/
|
||||
`sty`) is flag-neutral, so the reorder alone would work there, but the stores
|
||||
and the m68k `move`-based `LOADHR` are not, so it does not generalize.
|
||||
|
||||
## 5. Edge cases
|
||||
|
||||
- `void` targets: excluded from both the wrap condition and the per-pair
|
||||
handling, as today.
|
||||
- Mixed status flag + `@A`/`@D0` + float returns: covered by the same outer
|
||||
save/restore.
|
||||
- `amiga500` (68000): remains a hard codegen error, now triggered only for the
|
||||
flags-plus-register combination.
|
||||
- Portability note: `move ccr,-(sp)` / `move (sp)+,ccr` are 68010+ instructions.
|
||||
The only non-68000 m68k target is `qemu68k` (68020), so this is safe today. If
|
||||
a 68010 target is ever added it still works; a 68000 target cannot use it at
|
||||
all (already an error).
|
||||
|
||||
## 6. Test plan
|
||||
|
||||
- **ksim65 regression** (`compiler/test/codegeneration/TestNewCodegenBugs.kt`):
|
||||
`-> bool @Pc, ubyte @A` returning 42, assert the byte survives; run with
|
||||
`newCodegen=true` and with the legacy codegen as the reference, for
|
||||
`optimize` false and true.
|
||||
- **m68k**: a `TestM68k`-style test asserting the `move ccr,-(sp)` sandwich and
|
||||
that no `d0` round-trip remains around the extraction; if feasible, a
|
||||
`qemu68k` execution check.
|
||||
- **VM**: an IR-text test asserting the reorder/save appears (asmsub bodies
|
||||
cannot be executed by the VM).
|
||||
|
||||
## 7. Affected files
|
||||
|
||||
- `codeGenIntermediate/src/prog8/codegen/intermediate/AssignmentGen.kt` - reorder
|
||||
+ outer `PUSHST`/`POPST`.
|
||||
- `codeGenM68k/src/prog8/codegen/m68k/InstrControl.kt` - register-neutral
|
||||
`PUSHST`/`POPST`.
|
||||
- `compiler/test/codegeneration/TestNewCodegenBugs.kt`,
|
||||
`compiler/test/codegeneration/TestM68k.kt`,
|
||||
`compiler/test/vm/TestCompilerVirtual.kt` - tests.
|
||||
Reference in new issue
Block a user