From c2cf85c342e9a77446b1189cc911abc1c2804577 Mon Sep 17 00:00:00 2001 From: Jamie Willis Date: Sat, 4 Jan 2025 14:19:21 +0000 Subject: [PATCH] chore: added notes about why DynCall cannot suppress results --- .../deepembedding/backend/SequenceEmbedding.scala | 11 ++++++----- .../deepembedding/backend/StrictParsley.scala | 4 ++-- .../machine/instructions/PrimitiveInstrs.scala | 13 ++++++++----- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/SequenceEmbedding.scala b/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/SequenceEmbedding.scala index 4cda3f864..e40277943 100644 --- a/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/SequenceEmbedding.scala +++ b/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/SequenceEmbedding.scala @@ -94,12 +94,13 @@ private [deepembedding] final class >>=[A, B](val p: StrictParsley[A], private [ override def codeGen[M[_, +_]: ContOps, R](producesResults: Boolean)(implicit instrs: InstrBuffer, state: CodeGenState): M[R, Unit] = { suspend(p.codeGen[M, R](producesResults = true)) |> { instrs += instructions.DynCall[A] { x => - val p = f(x) - // FIXME: suppress results within p, then can remove pop - p.demandCalleeSave(state.numRegs) - if (implicitly[ContOps[M]].isStackSafe) p.overflows() - p.instrs + val q = f(x) + q.demandCalleeSave(state.numRegs) + if (implicitly[ContOps[M]].isStackSafe) q.overflows() + q.instrs } + // NOTE: this cannot be removed, because `q`'s instructions are cached: there is no way to tell it + // to not produce results that doesn't stop it working later. Something like `p.flatten ~> p.flatten` will crash. if (!producesResults) instrs += instructions.Pop } } diff --git a/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/StrictParsley.scala b/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/StrictParsley.scala index 6eae2bd29..a45185e52 100644 --- a/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/StrictParsley.scala +++ b/parsley/shared/src/main/scala/parsley/internal/deepembedding/backend/StrictParsley.scala @@ -161,11 +161,11 @@ private [deepembedding] object StrictParsley { if (calleeSaveRequired && localRegs.nonEmpty) { val end = state.freshLabel() val calleeSave = state.freshLabel() - instrs += new instructions.Push(false) + instrs += new instructions.Push(false) // callee-save is not active instrs += new instructions.Label(calleeSave) instrs += new instructions.CalleeSave(end, localRegs, reqRegs, allocatedRegs, numRegsUsedByParent) bodyGen |> { - instrs += new instructions.Push(true) + instrs += new instructions.Push(true) // callee-save is active instrs += new instructions.Jump(calleeSave) instrs += new instructions.Label(end) } diff --git a/parsley/shared/src/main/scala/parsley/internal/machine/instructions/PrimitiveInstrs.scala b/parsley/shared/src/main/scala/parsley/internal/machine/instructions/PrimitiveInstrs.scala index a958bb9e1..5565d3717 100644 --- a/parsley/shared/src/main/scala/parsley/internal/machine/instructions/PrimitiveInstrs.scala +++ b/parsley/shared/src/main/scala/parsley/internal/machine/instructions/PrimitiveInstrs.scala @@ -156,12 +156,15 @@ private [internal] object Span extends Instr { // FIXME: this isn't re-entrant, the stack allows us to tell, but the persisted // references are shared across nested visits. Is the reference deallocation idea also screwy? -// perhaps deallocation only occurs when we have an empty restore stack? +// perhaps deallocation only occurs when we have an empty restore stack (I think this is true, +// because deallocation allows for the reference to be remapped later, but we always want the +// same mapping for a let-bound parser) +// TODO: unit test to demonstrate the above issue! // This instruction holds mutable state, but it is safe to do so, because it's always the first instruction of a DynCall. -private [parsley] final class CalleeSave(var label: Int, localRegs: Set[Ref[_]], reqSize: Int, slots: List[(Int, Int)], saveArray: Array[AnyRef]) +private [parsley] final class CalleeSave(var label: Int, localRefs: Set[Ref[_]], reqSize: Int, slots: List[(Int, Int)], saveArray: Array[AnyRef]) extends InstrWithLabel { - private def this(label: Int, localRegs: Set[Ref[_]], reqSize: Int, slots: List[Int]) = - this(label, localRegs, reqSize, slots.zipWithIndex, new Array[AnyRef](slots.length)) + private def this(label: Int, localRefs: Set[Ref[_]], reqSize: Int, slots: List[Int]) = + this(label, localRefs, reqSize, slots.zipWithIndex, new Array[AnyRef](slots.length)) // this filters out the slots to ensure we only do callee-save on registers that might exist in the parent def this(label: Int, localRefs: Set[Ref[_]], reqSize: Int, slots: List[Int], numRegsInContext: Int) = this(label, localRefs, reqSize, slots.takeWhile(_ < numRegsInContext)) @@ -191,7 +194,7 @@ private [parsley] final class CalleeSave(var label: Int, localRegs: Set[Ref[_]], } // This is the only way to get them reallocated on the next invocation // FIXME: I think this isn't thread-safe, because two flatMaps can simulataneously reallocate? - localRegs.foreach(_.deallocate()) + localRefs.foreach(_.deallocate()) } private def continue(ctx: Context): Unit = {