Get rid of 'overwrite' destination kind

It wasn't safe (computing the rval might invalidate the lval addr), and needlessly
complicating things (code was already building up intermediary results to work
around other unsafeties).

Issue #667

Author: marijnh

src/comp/middle/trans.rs

@@ -2112,6 +2112,7 @@ fn move_val(cx: @block_ctxt, action: copy_action, dst: ValueRef,
                              ty_to_str(tcx, t));
 }
 
+// FIXME[DPS] rename to store_temp_expr
 fn move_val_if_temp(cx: @block_ctxt, action: copy_action, dst: ValueRef,
                     src: lval_result, t: ty::t) -> @block_ctxt {
     // Lvals in memory are not temporaries. Copy them.
@@ -2213,7 +2214,7 @@ fn trans_unary(bcx: @block_ctxt, op: ast::unop, e: @ast::expr,
             let llety = T_ptr(type_of(ccx, e_sp, e_ty));
             body = PointerCast(bcx, body, llety);
         }
-        bcx = trans_expr_save_in(bcx, e, body, INIT);
+        bcx = trans_expr_save_in(bcx, e, body);
         revoke_clean(bcx, box);
         ret store_in_dest(bcx, box, dest);
       }
@@ -2255,8 +2256,7 @@ fn trans_expr_fn(bcx: @block_ctxt, f: ast::_fn, sp: span,
         trans_closure(sub_cx, sp, f, llfn, none, [], id, {|_fcx|});
       }
     };
-    let {bcx, val: addr} = get_dest_addr(bcx, dest);
-    fill_fn_pair(bcx, addr, llfn, env);
+    fill_fn_pair(bcx, get_dest_addr(dest), llfn, env);
     ret bcx;
 }
 
@@ -2357,19 +2357,18 @@ fn trans_assign_op(bcx: @block_ctxt, op: ast::binop, dst: @ast::expr,
       }
       _ { }
     }
-    let rhs_res = trans_expr(lhs_res.bcx, src);
+    let {bcx, val: rhs_val} = trans_expr(lhs_res.bcx, src);
     if ty::type_is_sequence(tcx, t) {
         alt op {
           ast::add. {
-            ret tvec::trans_append(rhs_res.bcx, t, lhs_res.val,
-                                   rhs_res.val);
+            ret tvec::trans_append(bcx, t, lhs_res.val, rhs_val);
           }
           _ { }
         }
     }
-    let lhs_val = load_if_immediate(rhs_res.bcx, lhs_res.val, t);
-    ret trans_eager_binop(rhs_res.bcx, op, lhs_val, t, rhs_res.val, t,
-                          overwrite(lhs_res.val, t));
+    
+    ret trans_eager_binop(bcx, op, Load(bcx, lhs_res.val), t, rhs_val, t,
+                          save_in(lhs_res.val));
 }
 
 fn autoderef(cx: @block_ctxt, v: ValueRef, t: ty::t) -> result_t {
@@ -2485,7 +2484,6 @@ tag dest {
     by_val(@mutable ValueRef);
     by_ref(@mutable ValueRef);
     save_in(ValueRef);
-    overwrite(ValueRef, ty::t);
     ignore;
 }
 
@@ -2538,19 +2536,12 @@ fn store_in_dest(bcx: @block_ctxt, val: ValueRef, dest: dest) -> @block_ctxt {
       ignore. {}
       by_val(cell) { *cell = val; }
       save_in(addr) { Store(bcx, val, addr); }
-      overwrite(addr, tp) {
-        bcx = drop_ty(bcx, addr, tp);
-        Store(bcx, val, addr);
-      }
     }
     ret bcx;
 }
 
-fn get_dest_addr(bcx: @block_ctxt, dest: dest) -> result {
-    alt dest {
-      save_in(a) { rslt(bcx, a) }
-      overwrite(a, t) { rslt(drop_ty(bcx, a, t), a) }
-    }
+fn get_dest_addr(dest: dest) -> ValueRef {
+    alt dest { save_in(a) { a } }
 }
 
 // Wrapper through which legacy non-DPS code can use DPS functions
@@ -2733,7 +2724,7 @@ fn build_environment(bcx: @block_ctxt, lltydescs: [ValueRef],
         bcx = bound.bcx;
         alt bv {
           env_expr(e) {
-            bcx = trans_expr_save_in(bcx, e, bound.val, INIT);
+            bcx = trans_expr_save_in(bcx, e, bound.val);
             add_clean_temp_mem(bcx, bound.val, bound_tys[i]);
             temp_cleanups += [bound.val];
           }
@@ -3698,8 +3689,7 @@ fn trans_bind_1(cx: @block_ctxt, outgoing_fty: ty::t,
         let lv = lval_maybe_callee_to_lval(f_res, pair_ty);
         bcx = lv.bcx;
         // FIXME[DPS] factor this out
-        let {bcx, val: addr} = get_dest_addr(bcx, dest);
-        ret memmove_ty(bcx, addr, lv.val, pair_ty);
+        ret memmove_ty(bcx, get_dest_addr(dest), lv.val, pair_ty);
     }
     let closure = alt f_res.env {
       null_env. { none }
@@ -3736,8 +3726,7 @@ fn trans_bind_1(cx: @block_ctxt, outgoing_fty: ty::t,
                          closure.ptrty, ty_param_count, target_res);
 
     // Fill the function pair
-    let {bcx, val: addr} = get_dest_addr(bcx, dest);
-    fill_fn_pair(bcx, addr, llthunk.val, closure.ptr);
+    fill_fn_pair(bcx, get_dest_addr(dest), llthunk.val, closure.ptr);
     ret bcx;
 }
 
@@ -3845,7 +3834,7 @@ fn trans_args(cx: @block_ctxt, outer_cx: @block_ctxt, llenv: ValueRef,
         } else { alloca(cx, llretty) }
       }
       save_in(dst) { dst }
-      overwrite(_, _) | by_val(_) { alloca(cx, llretty) }
+      by_val(_) { alloca(cx, llretty) }
       by_ref(_) { dest_ref = true; alloca(cx, T_ptr(llretty)) }
     };
     // FIXME[DSP] does this always hold?
@@ -3970,10 +3959,6 @@ fn trans_call(in_cx: @block_ctxt, f: @ast::expr,
         }
       }
       save_in(_) { } // Already saved by callee
-      overwrite(a, t) {
-        bcx = drop_ty(bcx, a, t);
-        bcx = memmove_ty(bcx, a, llretslot, ret_ty);
-      }
       by_ref(cell) | by_val(cell) {
         *cell = Load(bcx, llretslot);
       }
@@ -4167,55 +4152,38 @@ fn trans_landing_pad(bcx: @block_ctxt,
 fn trans_tup(bcx: @block_ctxt, elts: [@ast::expr], id: ast::node_id,
              dest: dest) -> @block_ctxt {
     let t = node_id_type(bcx.fcx.lcx.ccx, id);
-    let (addr, overwrite) = alt dest {
+    let addr = alt dest {
       ignore. {
         for ex in elts { bcx = trans_expr_dps(bcx, ex, ignore); }
         ret bcx;
       }
-      save_in(pos) { (pos, none) }
-      overwrite(pos, _) {
-        let scratch = alloca(bcx, llvm::LLVMGetElementType(val_ty(pos)));
-        (scratch, some(pos))
-      }
+      save_in(pos) { pos }
     };
     let temp_cleanups = [], i = 0;
     for e in elts {
         let dst = GEP_tup_like_1(bcx, t, addr, [0, i]);
         let e_ty = ty::expr_ty(bcx_tcx(bcx), e);
-        bcx = trans_expr_save_in(dst.bcx, e, dst.val, INIT);
+        bcx = trans_expr_save_in(dst.bcx, e, dst.val);
         add_clean_temp_mem(bcx, dst.val, e_ty);
         temp_cleanups += [dst.val];
         i += 1;
     }
     for cleanup in temp_cleanups { revoke_clean(bcx, cleanup); }
-    alt overwrite {
-      some(pos) {
-        bcx = drop_ty(bcx, pos, t);
-        bcx = memmove_ty(bcx, pos, addr, t);
-      }
-      none. {}
-    }
     ret bcx;
 }
 
 fn trans_rec(bcx: @block_ctxt, fields: [ast::field],
              base: option::t<@ast::expr>, id: ast::node_id,
              dest: dest) -> @block_ctxt {
     let t = node_id_type(bcx_ccx(bcx), id);
-    let (addr, overwrite) = alt dest {
+    let addr = alt dest {
       ignore. {
         for fld in fields {
             bcx = trans_expr_dps(bcx, fld.node.expr, ignore);
         }
         ret bcx;
       }
-      save_in(pos) { (pos, none) }
-      // The expressions that populate the fields might still use the old
-      // record, so we build the new on in a scratch area
-      overwrite(pos, _) {
-        let scratch = alloca(bcx, llvm::LLVMGetElementType(val_ty(pos)));
-        (scratch, some(pos))
-      }
+      save_in(pos) { pos }
     };
 
     let base_val = alt base {
@@ -4234,7 +4202,7 @@ fn trans_rec(bcx: @block_ctxt, fields: [ast::field],
         bcx = dst.bcx;
         alt vec::find({|f| str::eq(f.node.ident, tf.ident)}, fields) {
           some(f) {
-            bcx = trans_expr_save_in(bcx, f.node.expr, dst.val, INIT);
+            bcx = trans_expr_save_in(bcx, f.node.expr, dst.val);
           }
           none. {
             let base = GEP_tup_like_1(bcx, t, base_val, [0, i]);
@@ -4249,13 +4217,6 @@ fn trans_rec(bcx: @block_ctxt, fields: [ast::field],
     // Now revoke the cleanups as we pass responsibility for the data
     // structure on to the caller
     for cleanup in temp_cleanups { revoke_clean(bcx, cleanup); }
-    alt overwrite {
-      some(pos) {
-        bcx = drop_ty(bcx, pos, t);
-        bcx = memmove_ty(bcx, pos, addr, t);
-      }
-      none. {}
-    }
     ret bcx;
 }
 
@@ -4274,19 +4235,37 @@ fn trans_expr(cx: @block_ctxt, e: @ast::expr) -> result {
     }
 }
 
-fn trans_expr_save_in(bcx: @block_ctxt, e: @ast::expr, dest: ValueRef,
-                      kind: copy_action) -> @block_ctxt {
+fn trans_expr_save_in(bcx: @block_ctxt, e: @ast::expr, dest: ValueRef)
+    -> @block_ctxt {
     let tcx = bcx_tcx(bcx), t = ty::expr_ty(tcx, e);
     let dst = if ty::type_is_bot(tcx, t) || ty::type_is_nil(tcx, t) {
         ignore
-    } else if kind == INIT {
-        save_in(dest)
-    } else {
-        overwrite(dest, t)
-    };
+    } else { save_in(dest) };
     ret trans_expr_dps(bcx, e, dst);
 }
 
+fn trans_temp_expr(bcx: @block_ctxt, e: @ast::expr) -> lval_result {
+    if expr_is_lval(bcx_tcx(bcx), e) {
+        ret trans_lval(bcx, e);
+    } else {
+        let tcx = bcx_tcx(bcx);
+        let ty = ty::expr_ty(tcx, e);
+        if ty::type_is_nil(tcx, ty) || ty::type_is_bot(tcx, ty) {
+            bcx = trans_expr_dps(bcx, e, ignore);
+            ret {bcx: bcx, val: C_nil(), is_mem: false};
+        } else if type_is_immediate(bcx_ccx(bcx), ty) {
+            let cell = empty_dest_cell();
+            bcx = trans_expr_dps(bcx, e, by_val(cell));
+            ret {bcx: bcx, val: *cell, is_mem: false};
+        } else {
+            let {bcx, val: scratch} = alloc_ty(bcx, ty);
+            bcx = trans_expr_dps(bcx, e, save_in(scratch));
+            ret {bcx: bcx, val: scratch, is_mem: false};
+        }
+    }
+}
+
+// FIXME[DPS] supersede by trans_temp_expr, get rid of by_ref dests
 fn trans_expr_by_ref(bcx: @block_ctxt, e: @ast::expr) -> result {
     let cell = empty_dest_cell();
     bcx = trans_expr_dps(bcx, e, by_ref(cell));
@@ -4430,22 +4409,20 @@ fn trans_expr_dps(bcx: @block_ctxt, e: @ast::expr, dest: dest)
       }
       ast::expr_assign(dst, src) {
         assert dest == ignore;
-        let {bcx, val: lhs_addr, is_mem} = trans_lval(bcx, dst);
+        let src_r = trans_temp_expr(bcx, src);
+        let {bcx, val: addr, is_mem} = trans_lval(src_r.bcx, dst);
         assert is_mem;
-        ret trans_expr_save_in(bcx, src, lhs_addr, DROP_EXISTING);
+        ret move_val_if_temp(bcx, DROP_EXISTING, addr, src_r,
+                             ty::expr_ty(bcx_tcx(bcx), src));
       }
       ast::expr_move(dst, src) {
+        // FIXME: calculate copy init-ness in typestate.
         assert dest == ignore;
-        let {bcx, val: addr, is_mem} = trans_lval(bcx, dst);
+        let src_r = trans_temp_expr(bcx, src);
+        let {bcx, val: addr, is_mem} = trans_lval(src_r.bcx, dst);
         assert is_mem;
-        // FIXME: calculate copy init-ness in typestate.
-        if expr_is_lval(tcx, src) {
-            ret trans_expr_save_in(bcx, src, addr, DROP_EXISTING);
-        } else {
-            let srclv = trans_lval(bcx, src);
-            let t = ty::expr_ty(tcx, src);
-            ret move_val(srclv.bcx, DROP_EXISTING, addr, srclv, t);
-        }
+        ret move_val(bcx, DROP_EXISTING, addr, src_r,
+                     ty::expr_ty(bcx_tcx(bcx), src));
       }
       ast::expr_swap(dst, src) {
         assert dest == ignore;
@@ -4492,9 +4469,6 @@ fn lval_to_dps(bcx: @block_ctxt, e: @ast::expr, dest: dest) -> @block_ctxt {
         *cell = val;
       }
       save_in(loc) { bcx = move_val_if_temp(bcx, INIT, loc, lv, ty); }
-      overwrite(loc, _) {
-        bcx = move_val_if_temp(bcx, DROP_EXISTING, loc, lv, ty);
-      }
       ignore. {}
     }
     ret bcx;
@@ -4746,7 +4720,7 @@ fn trans_ret(bcx: @block_ctxt, e: option::t<@ast::expr>) -> @block_ctxt {
             Store(cx, val, bcx.fcx.llretptr);
             bcx = cx;
         } else {
-            bcx = trans_expr_save_in(bcx, x, bcx.fcx.llretptr, INIT);
+            bcx = trans_expr_save_in(bcx, x, bcx.fcx.llretptr);
         }
       }
       _ {}
@@ -4784,7 +4758,7 @@ fn init_local(bcx: @block_ctxt, local: @ast::local) -> @block_ctxt {
       some(init) {
         if init.op == ast::init_assign ||
            !expr_is_lval(bcx_tcx(bcx), init.expr) {
-            bcx = trans_expr_save_in(bcx, init.expr, llptr, INIT);
+            bcx = trans_expr_save_in(bcx, init.expr, llptr);
         } else { // This is a move from an lval, must perform an actual move
             let sub = trans_lval(bcx, init.expr);
             bcx = move_val(sub.bcx, INIT, llptr, sub, ty);

src/comp/middle/trans_objects.rs

@@ -378,7 +378,7 @@ fn trans_anon_obj(bcx: @block_ctxt, sp: span, anon_obj: ast::anon_obj,
         revoke_clean(bcx, box);
         box = PointerCast(bcx, box, llbox_ty);
     }
-    let {bcx, val: pair} = trans::get_dest_addr(bcx, dest);
+    let pair = trans::get_dest_addr(dest);
     let pair_vtbl = GEP(bcx, pair, [C_int(0), C_int(abi::obj_field_vtbl)]);
     Store(bcx, vtbl, pair_vtbl);
     let pair_box = GEP(bcx, pair, [C_int(0), C_int(abi::obj_field_box)]);

src/comp/middle/trans_uniq.rs

@@ -29,7 +29,7 @@ fn trans_uniq(bcx: @block_ctxt, contents: @ast::expr,
     check type_is_unique_box(bcx, uniq_ty);
     let {bcx, val: llptr} = alloc_uniq(bcx, uniq_ty);
     add_clean_free(bcx, llptr, true);
-    bcx = trans::trans_expr_save_in(bcx, contents, llptr, INIT);
+    bcx = trans::trans_expr_save_in(bcx, contents, llptr);
     revoke_clean(bcx, llptr);
     ret trans::store_in_dest(bcx, llptr, dest);
 }

src/comp/middle/trans_vec.rs

@@ -124,17 +124,13 @@ fn trans_vec(bcx: @block_ctxt, args: [@ast::expr], id: ast::node_id,
         let lleltptr = if ty::type_has_dynamic_size(bcx_tcx(bcx), unit_ty) {
             InBoundsGEP(bcx, dataptr, [Mul(bcx, C_uint(i), llunitsz)])
         } else { InBoundsGEP(bcx, dataptr, [C_uint(i)]) };
-        bcx = trans::trans_expr_save_in(bcx, e, lleltptr, INIT);
+        bcx = trans::trans_expr_save_in(bcx, e, lleltptr);
         add_clean_temp_mem(bcx, lleltptr, unit_ty);
         temp_cleanups += [lleltptr];
         i += 1u;
     }
     for clean in temp_cleanups { revoke_clean(bcx, clean); }
-    let vptrptr = alt dest {
-      trans::save_in(a) { a }
-      trans::overwrite(a, t) { bcx = trans::drop_ty(bcx, a, t); a }
-    };
-    Store(bcx, vptr, vptrptr);
+    Store(bcx, vptr, trans::get_dest_addr(dest));
     ret bcx;
 }
 
@@ -147,11 +143,7 @@ fn trans_str(bcx: @block_ctxt, s: str, dest: dest) -> @block_ctxt {
     let bcx =
         call_memmove(bcx, get_dataptr_simple(bcx, sptr, T_i8()), llcstr,
                      C_uint(veclen)).bcx;
-    let sptrptr = alt dest {
-      trans::save_in(a) { a }
-      trans::overwrite(a, t) { bcx = trans::drop_ty(bcx, a, t); a }
-    };
-    Store(bcx, sptr, sptrptr);
+    Store(bcx, sptr, trans::get_dest_addr(dest));
     ret bcx;
 }
 
@@ -266,13 +258,7 @@ fn trans_add(bcx: @block_ctxt, vec_ty: ty::t, lhsptr: ValueRef,
 
     let bcx = iter_vec_raw(bcx, lhsptr, vec_ty, lhs_fill, copy_fn);
     bcx = iter_vec_raw(bcx, rhsptr, vec_ty, rhs_fill, copy_fn);
-    alt dest {
-      trans::save_in(a) { Store(bcx, new_vec_ptr, a); }
-      trans::overwrite(a, t) {
-        bcx = trans::drop_ty(bcx, a, t);
-        Store(bcx, new_vec_ptr, a);
-      }
-    }
+    Store(bcx, new_vec_ptr, trans::get_dest_addr(dest));
     ret bcx;
 }