Skip to content

Commit

Permalink
Fix post-stmt drop calculations. Closes #106.
Browse files Browse the repository at this point in the history
  • Loading branch information
graydon committed Jul 12, 2010
1 parent 30c4070 commit b3cf793
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ TEST_XFAILS_LLVM := $(addprefix test/run-pass/, \
vec-append.rs \
vec-concat.rs \
vec-drop.rs \
vec-late-init.rs \
vec-slice.rs \
vec.rs \
writealias.rs \
Expand Down
40 changes: 30 additions & 10 deletions src/boot/me/typestate.ml
Original file line number Diff line number Diff line change
Expand Up @@ -988,23 +988,30 @@ let lifecycle_visitor
* used later on in translation.
*)

let (live_block_slots:(node_id Stack.t) Stack.t) = Stack.create () in
let (live_block_slots:(node_id, unit) Hashtbl.t) = Hashtbl.create 0 in
let (block_slots:(node_id Stack.t) Stack.t) = Stack.create () in

let (implicit_init_block_slots:(node_id,node_id) Hashtbl.t) =
Hashtbl.create 0
in

let push_slot sl =
Stack.push sl (Stack.top block_slots)
in

let mark_slot_init sl =
Stack.push sl (Stack.top live_block_slots)
Hashtbl.replace live_block_slots sl ()
in


let visit_block_pre b =
Stack.push (Stack.create()) live_block_slots;
Stack.push (Stack.create()) block_slots;
begin
match htab_search implicit_init_block_slots b.id with
None -> ()
| Some slot -> mark_slot_init slot
| Some slot ->
push_slot slot;
mark_slot_init slot
end;
inner.Walk.visit_block_pre b
in
Expand All @@ -1026,7 +1033,7 @@ let lifecycle_visitor

let visit_block_post b =
inner.Walk.visit_block_post b;
let blk_live = Stack.pop live_block_slots in
let blk_slots = Stack.pop block_slots in
let stmts = b.node in
let len = Array.length stmts in
if len > 0
Expand All @@ -1038,8 +1045,13 @@ let lifecycle_visitor
| Ast.STMT_be _ ->
() (* Taken care of in visit_stmt_post below. *)
| _ ->
let slots = stk_elts_from_top blk_live in
note_drops s slots
let slots = stk_elts_from_bot blk_slots in

This comment has been minimized.

Copy link
@jyasskin

jyasskin Jul 12, 2010

Contributor

Why'd you reverse the order of the slots list?

This comment has been minimized.

Copy link
@graydon

graydon Jul 12, 2010

Author Contributor

Because the old order (top-to-bottom drop) was wrong. It happened to work because there's only one test in the suite which is sensitive to the difference -- mlist.rs -- and it was working top-to-bottom on the initialization order of slots, not the declaration order. Turns out the slots in that test happen to be initialized in bottom-to-top order, since they are the result of desugaring, but when I changed to driving the drops from the declarations, I had to change the order to match.

You can see the asymmetry if you run "rustboot -last test/run-pass/mlist.rs".

This comment has been minimized.

Copy link
@jyasskin

jyasskin Jul 12, 2010

Contributor

It'd be nice to have a comment somewhere saying the order is significant and what it means. To my naive eye, it looked like marking slots dead could be done in any order and still be correct. (I tried changing this back to top, and mlist still passed.) Ah, but it matters to the order that destructors run. I'll see if I can send you a test for that.

Sorry to bug you about this. I'm hoping I'm representative of the people who are reading this code, and so adding comments where I'm confused will help them too. But I'll stop if it's just wasting your time.

This comment has been minimized.

Copy link
@graydon

graydon Jul 12, 2010

Author Contributor

Mlist still passed! That's surprising. If I switch it back here it most certainly crashes here ... except of course if I look at the code once more with slightly less cold medicine in my body and more sleep, it does indeed also look wrong now: drop order on declarations should happen inside-out. So I was wallpapering over a bug. Sigh. I'll look at this more once I'm coffee'd and in the office with a decent monitor and valgrind.

No worries about the chattiness, I'm very happy to have someone with your attention to detail engaged in reading it.

let live =
List.filter
(fun i -> Hashtbl.mem live_block_slots i)
slots
in
note_drops s live

This comment has been minimized.

Copy link
@jyasskin

jyasskin Jul 12, 2010

Contributor

It seems weird that the "live" slots are the ones you drop. Am I just misunderstanding the purpose of "dropping" a slot?

This comment has been minimized.

Copy link
@graydon

graydon Jul 12, 2010

Author Contributor

This is in the block post-traverse visitor method, so it's happening as a set of live slots go out of scope.

This comment has been minimized.

Copy link
@jyasskin

jyasskin Jul 12, 2010

Contributor

I see. I think I was confusing a variable's live range, which would end at the last use, and the point at which a value should be destroyed if it has a destructor. Thanks.

end;
in

Expand Down Expand Up @@ -1081,6 +1093,9 @@ let lifecycle_visitor
init_lval lv_dst
end;

| Ast.STMT_decl (Ast.DECL_slot (_, sloti)) ->
push_slot sloti.id

| Ast.STMT_init_rec (lv_dst, _, _)
| Ast.STMT_init_tup (lv_dst, _)
| Ast.STMT_init_vec (lv_dst, _)
Expand Down Expand Up @@ -1117,9 +1132,14 @@ let lifecycle_visitor
match s.node with
Ast.STMT_ret _
| Ast.STMT_be _ ->
let stks = stk_elts_from_top live_block_slots in
let slots = List.concat (List.map stk_elts_from_top stks) in
note_drops s slots
let stks = stk_elts_from_top block_slots in
let slots = List.concat (List.map stk_elts_from_bot stks) in
let live =
List.filter
(fun i -> Hashtbl.mem live_block_slots i)
slots
in
note_drops s live
| _ -> ()
in

Expand Down
9 changes: 9 additions & 0 deletions src/test/run-pass/vec-late-init.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
fn main() {
let vec[int] later;
if (true) {
later = vec(1);
} else {
later = vec(2);
}
log later.(0);
}

0 comments on commit b3cf793

Please sign in to comment.