From 091f91e74ff843f8174634817af38e94e16acb41 Mon Sep 17 00:00:00 2001
From: Chris Fallin <chris@cfallin.org>
Date: Sun, 26 Jun 2022 14:09:41 -0700
Subject: [PATCH] Bugfix: no hole in liveranges for pinned vreg move src.

Right now, pinned vregs are a way of naming real registers (a
compatibility shim of sorts for Cranelift's `RealReg`s) and can be used
as sources and dests of moves. When the input program does so, regalloc2
converts these into "ghost" uses and defs on the other vreg of the move
(dest or source, respectively) with a fixed-register constraint. So
`move v128, v0` where `v0` is pinned to `p0` turns into a "ghost def"
on `v128` with constraint `fixed p0`.

There is some fancy manipulation of liveranges to make this all work
while properly recording where the preg's value must be preserved.
Unfortunately, there was an off-by-one in the location of the move and
transition of live-ranges which interacts poorly with the "implicit
live-in" of pinned vregs at function start. As a result, a function body
that starts like:

```
    move v128, v0
    def v9000
    move v129, v1
```

might allocate `p1` (to which `v1` is pinned) for `v9000`. This clobbers
the original value.

Fortunately this only impacts the implicit live-in, and Cranelift's use
of regalloc2 is such that it will always copy all values out of pinned
vregs (creating ghost defs) without intervening defs, *except* in the
case of `sret` ("structure return") arguments. If a program does not use
`sret` arguments (and the `cranelift-wasm` frontend does not), then this
bug should not be reachable.

Long-term, we really need to kill pinned vregs with fire (#3); the
special cases that arise from these, and from special handling of moves,
are too much incidental complexity. All of this can go away once
Cranelift migrates all fixed-register cases to operand constraints
instead. That will be a happy day.

Thanks to @bjorn3 for finding and reporting this issue!
---
 src/ion/liveranges.rs | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/ion/liveranges.rs b/src/ion/liveranges.rs
index 350002d6..df612ee4 100644
--- a/src/ion/liveranges.rs
+++ b/src/ion/liveranges.rs
@@ -589,7 +589,7 @@ impl<'a, F: Function> Env<'a, F> {
                                             src.vreg(),
                                             OperandKind::Def,
                                             OperandPos::Late,
-                                            ProgPoint::after(inst),
+                                            ProgPoint::before(inst),
                                         )
                                     } else {
                                         // Dest is pinned: this is a use on the src with a pinned preg.
@@ -681,18 +681,22 @@ impl<'a, F: Function> Env<'a, F> {
                                     if live.get(pinned_vreg.vreg()) {
                                         let pinned_lr = vreg_ranges[pinned_vreg.vreg()];
                                         let orig_start = self.ranges[pinned_lr.index()].range.from;
+                                        // Following instruction start
+                                        // (so we don't transition in
+                                        // middle of inst).
+                                        let new_start = ProgPoint::before(progpoint.inst().next());
                                         trace!(
                                             " -> live with LR {:?}; truncating to start at {:?}",
                                             pinned_lr,
-                                            progpoint.next()
+                                            new_start,
                                         );
-                                        self.ranges[pinned_lr.index()].range.from =
-                                            progpoint.next();
+                                        self.ranges[pinned_lr.index()].range.from = new_start;
+
                                         let new_lr = self.add_liverange_to_vreg(
                                             VRegIndex::new(pinned_vreg.vreg()),
                                             CodeRange {
                                                 from: orig_start,
-                                                to: progpoint.prev(),
+                                                to: progpoint,
                                             },
                                         );
                                         vreg_ranges[pinned_vreg.vreg()] = new_lr;
@@ -725,7 +729,7 @@ impl<'a, F: Function> Env<'a, F> {
                                                 VRegIndex::new(pinned_vreg.vreg()),
                                                 CodeRange {
                                                     from: self.cfginfo.block_entry[block.index()],
-                                                    to: ProgPoint::before(inst),
+                                                    to: progpoint,
                                                 },
                                             );
                                             vreg_ranges[pinned_vreg.vreg()] = new_lr;