Skip to content

Commit

Permalink
Fix bug in allocation of handlerOffset and handlerArgs
Browse files Browse the repository at this point in the history
MLton#321 (especially 69eec2d) introduced a new raising
convention, whereby raised values are communicated from the raisee to
the handler via the ML stack.  As noted in that commit: "Sufficient
space for raised values are reserved in the handler frame immediately
above the handler label."  However, the mechanism by which this space
was reserved was not correct and, in rare circumstances, could lead to
a `Machine.typeCheck` internal compiler error.

The intended mechanism was to allocate stack slots for the link,
handler label, and handler arguments after all of the stack slots
allocated to the incoming actual arguments.  The code to do so was
like this:

    (* create a stack allocation that includes all incoming actuals *)
    val stack = ...
    (* allocate stack slot for `link` *)
    val (stack, {offset = linkOffset, ...}) =
       Allocation.Stack.get (stack, Type.exnStack ())
    (* allocate stack slot for `handler` *)
    val (_, {offset = handlerOffset, ...}) =
       Allocation.Stack.get (stack, Type.label (Label.newNoname ()))
    (* align handler args immediately after handler *)
    val handlerArgsOffset =
       Bytes.align
       (Bytes.+ (handlerOffset, Runtime.labelSize ()),
        {alignment = (case !Control.align of
                         Control.Align4 => Bytes.inWord32
                       | Control.Align8 => Bytes.inWord64)})
    (* reset `handlerOffset` to immediately before handler args *)
    val handlerOffset = Bytes.- (handlerArgsOffset, Runtime.labelSize ())

where `stack` was the stack allocation of incoming actual arguments.
Typically, this sequence would allocate after all of the incoming
actual arguments.  However `Allocation.Stack.get` does try to find
"gaps" in an allocation in which to allocate new stack slots, in order
to keep frame sizes small.

An issue arises on 32-bit platforms, when `Type.exnStack()` and
`Type.label` are 32-bits but `Real64` values are 8-byte aligned.  An
RSSA IR function with `Real64` arguments may have 32-bit "gaps" in
order to place the next `Real64` argument on an 8-byte boundary.  In
which case, the above could place the `linkOffset` and (initial)
`handlerOffset` in between actual arguments.  If those offsets were
only used to hold 32-bit values, then there would be no problem.  But,
the `handlerArgsOffset` is computed from `handlerOffset` and in the
previously described scenario, would interfere with actual
arguments (as would the reset `handlerOffset`).

In practice, this may not have caused problems, if the actual
arguments were dead before writing to `handlerOffset` (at the point of
installing an exception handler) or to `handlerArgsOffset` (at the
point of a raise back to this function).  But, it would be flagged as
a Machine IR type error.

The fix is to simply ensure that `handlerOffset` really is placed
after all of the incoming arguments:

    val handlerOffset =
       Type.align (Type.label (Label.newNoname ()), Allocation.Stack.size stack)
  • Loading branch information
MatthewFluet committed Nov 9, 2023
1 parent 9c8c871 commit 823ec5f
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions mlton/backend/allocate-variables.fun
Original file line number Diff line number Diff line change
Expand Up @@ -463,8 +463,8 @@ fun allocate {function = f: Rssa.Function.t,
*)
val (stack, {offset = linkOffset, ...}) =
Allocation.Stack.get (stack, Type.exnStack ())
val (_, {offset = handlerOffset, ...}) =
Allocation.Stack.get (stack, Type.label (Label.newNoname ()))
val handlerTy = Type.label (Label.newNoname ())
val handlerOffset = Type.align (handlerTy, Allocation.Stack.size stack)
val handlerArgsOffset =
Bytes.align
(Bytes.+ (handlerOffset, Runtime.labelSize ()),
Expand Down

0 comments on commit 823ec5f

Please sign in to comment.