From aa364ddaca12c327360abbface95049eb9431d45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Uwe=20Kr=C3=BCger?= <45282134+UweKrueger@users.noreply.github.com> Date: Mon, 13 Jul 2020 12:19:28 +0200 Subject: [PATCH] checker, cgen: checks for shared/lock, first autolock (#5815) --- vlib/v/ast/ast.v | 1 + vlib/v/checker/checker.v | 70 ++++++++++++++++--- vlib/v/checker/tests/lock_already_locked.out | 7 ++ vlib/v/checker/tests/lock_already_locked.vv | 16 +++++ vlib/v/checker/tests/lock_already_rlocked.out | 7 ++ vlib/v/checker/tests/lock_already_rlocked.vv | 16 +++++ vlib/v/checker/tests/lock_const.out | 7 ++ vlib/v/checker/tests/lock_const.vv | 11 +++ vlib/v/checker/tests/lock_needed.out | 7 ++ vlib/v/checker/tests/lock_needed.vv | 12 ++++ vlib/v/checker/tests/lock_nonshared.out | 7 ++ vlib/v/checker/tests/lock_nonshared.vv | 14 ++++ vlib/v/gen/cgen.v | 7 ++ vlib/v/tests/autolock_array_1.v | 52 ++++++++++++++ 14 files changed, 223 insertions(+), 11 deletions(-) create mode 100644 vlib/v/checker/tests/lock_already_locked.out create mode 100644 vlib/v/checker/tests/lock_already_locked.vv create mode 100644 vlib/v/checker/tests/lock_already_rlocked.out create mode 100644 vlib/v/checker/tests/lock_already_rlocked.vv create mode 100644 vlib/v/checker/tests/lock_const.out create mode 100644 vlib/v/checker/tests/lock_const.vv create mode 100644 vlib/v/checker/tests/lock_needed.out create mode 100644 vlib/v/checker/tests/lock_needed.vv create mode 100644 vlib/v/checker/tests/lock_nonshared.out create mode 100644 vlib/v/checker/tests/lock_nonshared.vv create mode 100644 vlib/v/tests/autolock_array_1.v diff --git a/vlib/v/ast/ast.v b/vlib/v/ast/ast.v index c26c4bc6a6..4ae20042bc 100644 --- a/vlib/v/ast/ast.v +++ b/vlib/v/ast/ast.v @@ -396,6 +396,7 @@ pub: pub mut: left_type table.Type right_type table.Type + auto_locked string } pub struct PostfixExpr { diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index d0deea50b6..d430415797 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -31,6 +31,8 @@ pub mut: const_decl string const_deps []string const_names []string + locked_names []string // vars that are currently locked + rlocked_names []string // vars that are currently read-locked pref &pref.Preferences // Preferences shared from V struct in_for_count int // if checker is currently in an for loop // checked_ident string // to avoid infinit checker loops @@ -571,7 +573,7 @@ pub fn (mut c Checker) infix_expr(mut infix_expr ast.InfixExpr) table.Type { .left_shift { if left.kind == .array { // `array << elm` - c.fail_if_immutable(infix_expr.left) + infix_expr.auto_locked, _ = c.fail_if_immutable(infix_expr.left) left_value_type := c.table.value_type(left_type) left_value_sym := c.table.get_type_symbol(left_value_type) if left_value_sym.kind == .interface_ { @@ -656,11 +658,15 @@ pub fn (mut c Checker) infix_expr(mut infix_expr ast.InfixExpr) table.Type { } } -fn (mut c Checker) fail_if_immutable(expr ast.Expr) { +// returns name and position of variable that needs write lock +fn (mut c Checker) fail_if_immutable(expr ast.Expr) (string, token.Position) { + mut to_lock := '' // name of variable that needs lock + mut pos := token.Position{} // and its position + mut explicit_lock_needed := false match expr { ast.CastExpr { // TODO - return + return '', pos } ast.Ident { scope := c.file.scope.innermost(expr.pos.pos) @@ -670,24 +676,30 @@ fn (mut c Checker) fail_if_immutable(expr ast.Expr) { expr.pos) } v.is_changed = true + if v.typ.share() == .shared_t { + if expr.name !in c.locked_names { + to_lock = expr.name + pos = expr.pos + } + } } else if expr.name in c.const_names { c.error('cannot modify constant `$expr.name`', expr.pos) } } ast.IndexExpr { - c.fail_if_immutable(expr.left) + to_lock, pos = c.fail_if_immutable(expr.left) } ast.ParExpr { - c.fail_if_immutable(expr.expr) + to_lock, pos = c.fail_if_immutable(expr.expr) } ast.PrefixExpr { - c.fail_if_immutable(expr.right) + to_lock, pos = c.fail_if_immutable(expr.right) } ast.SelectorExpr { // retrieve table.Field if expr.expr_type == 0 { c.error('0 type in SelectorExpr', expr.pos) - return + return '', pos } typ_sym := c.table.get_type_symbol(c.unwrap_generic(expr.expr_type)) match typ_sym.kind { @@ -696,14 +708,18 @@ fn (mut c Checker) fail_if_immutable(expr ast.Expr) { field_info := struct_info.find_field(expr.field_name) or { type_str := c.table.type_to_str(expr.expr_type) c.error('unknown field `${type_str}.$expr.field_name`', expr.pos) - return + return '', pos } if !field_info.is_mut { type_str := c.table.type_to_str(expr.expr_type) c.error('field `$expr.field_name` of struct `$type_str` is immutable', expr.pos) } - c.fail_if_immutable(expr.expr) + to_lock, pos = c.fail_if_immutable(expr.expr) + if to_lock != '' { + // No automatic lock for struct access + explicit_lock_needed = true + } } .array, .string { // This should only happen in `builtin` @@ -721,18 +737,27 @@ fn (mut c Checker) fail_if_immutable(expr ast.Expr) { ast.CallExpr { // TODO: should only work for builtin method if expr.name == 'slice' { - return + to_lock, pos = c.fail_if_immutable(expr.left) + if to_lock != '' { + // No automatic lock for array slicing (yet(?)) + explicit_lock_needed = true + } } else { c.error('cannot use function call as mut', expr.pos) } } ast.ArrayInit { - return + return '', pos } else { c.error('unexpected expression `${typeof(expr)}`', expr.position()) } } + if explicit_lock_needed { + c.error('`$to_lock` is `shared` and needs explicit lock for `${typeof(expr)}`', pos) + to_lock = '' + } + return to_lock, pos } pub fn (mut c Checker) call_expr(mut call_expr ast.CallExpr) table.Type { @@ -2626,10 +2651,33 @@ fn (mut c Checker) match_exprs(mut node ast.MatchExpr, type_sym table.TypeSymbol } pub fn (mut c Checker) lock_expr(mut node ast.LockExpr) table.Type { + scope := c.file.scope.innermost(node.pos.pos) for id in node.lockeds { c.ident(mut id) + if v := scope.find_var(id.name) { + if v.typ.share() != .shared_t { + c.error('`$id.name` must be declared `shared` to be locked', id.pos) + } + } else { + c.error('`$id.name` is not a variable and cannot be locked', id.pos) + } + if id.name in c.locked_names { + c.error('`$id.name` is already locked', id.pos) + } else if id.name in c.rlocked_names { + c.error('`$id.name` is already read-locked', id.pos) + } + if node.is_rlock { + c.rlocked_names << id.name + } else { + c.locked_names << id.name + } } c.stmts(node.stmts) + if node.is_rlock { + c.rlocked_names = c.rlocked_names[.. c.rlocked_names.len - node.lockeds.len] + } else { + c.locked_names = c.locked_names[.. c.locked_names.len - node.lockeds.len] + } // void for now... maybe sometime `x := lock a { a.getval() }` return table.void_type } diff --git a/vlib/v/checker/tests/lock_already_locked.out b/vlib/v/checker/tests/lock_already_locked.out new file mode 100644 index 0000000000..b1e7286a7a --- /dev/null +++ b/vlib/v/checker/tests/lock_already_locked.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/lock_already_locked.v:11:9: error: `a` is already locked + 9 | } + 10 | lock a { + 11 | rlock a { + | ^ + 12 | a.x++ + 13 | } diff --git a/vlib/v/checker/tests/lock_already_locked.vv b/vlib/v/checker/tests/lock_already_locked.vv new file mode 100644 index 0000000000..7bb636578f --- /dev/null +++ b/vlib/v/checker/tests/lock_already_locked.vv @@ -0,0 +1,16 @@ +struct St { +mut: + x int +} + +fn main() { + shared a := &St{ + x: 5 + } + lock a { + rlock a { + a.x++ + } + } + println(a.x) +} diff --git a/vlib/v/checker/tests/lock_already_rlocked.out b/vlib/v/checker/tests/lock_already_rlocked.out new file mode 100644 index 0000000000..9878bf23c1 --- /dev/null +++ b/vlib/v/checker/tests/lock_already_rlocked.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/lock_already_rlocked.v:11:8: error: `a` is already read-locked + 9 | } + 10 | rlock a { + 11 | lock a { + | ^ + 12 | a.x++ + 13 | } diff --git a/vlib/v/checker/tests/lock_already_rlocked.vv b/vlib/v/checker/tests/lock_already_rlocked.vv new file mode 100644 index 0000000000..c8e823674a --- /dev/null +++ b/vlib/v/checker/tests/lock_already_rlocked.vv @@ -0,0 +1,16 @@ +struct St { +mut: + x int +} + +fn main() { + shared a := &St{ + x: 5 + } + rlock a { + lock a { + a.x++ + } + } + println(a.x) +} diff --git a/vlib/v/checker/tests/lock_const.out b/vlib/v/checker/tests/lock_const.out new file mode 100644 index 0000000000..8d55659fa0 --- /dev/null +++ b/vlib/v/checker/tests/lock_const.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/lock_const.v:7:8: error: `a` is not a variable and cannot be locked + 5 | fn main() { + 6 | mut c := 0 + 7 | rlock a { + | ^ + 8 | c = a + 9 | } diff --git a/vlib/v/checker/tests/lock_const.vv b/vlib/v/checker/tests/lock_const.vv new file mode 100644 index 0000000000..df16a6afe5 --- /dev/null +++ b/vlib/v/checker/tests/lock_const.vv @@ -0,0 +1,11 @@ +const ( + a = 5 +) + +fn main() { + mut c := 0 + rlock a { + c = a + } + println(c) +} diff --git a/vlib/v/checker/tests/lock_needed.out b/vlib/v/checker/tests/lock_needed.out new file mode 100644 index 0000000000..dd7855d72d --- /dev/null +++ b/vlib/v/checker/tests/lock_needed.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/lock_needed.v:10:2: error: `abc` is `shared` and needs explicit lock for `v.ast.SelectorExpr` + 8 | x: 5 + 9 | } + 10 | abc.x++ + | ~~~ + 11 | println(abc.x) + 12 | } diff --git a/vlib/v/checker/tests/lock_needed.vv b/vlib/v/checker/tests/lock_needed.vv new file mode 100644 index 0000000000..805e790728 --- /dev/null +++ b/vlib/v/checker/tests/lock_needed.vv @@ -0,0 +1,12 @@ +struct St { +mut: + x int +} + +fn main() { + shared abc := &St{ + x: 5 + } + abc.x++ + println(abc.x) +} diff --git a/vlib/v/checker/tests/lock_nonshared.out b/vlib/v/checker/tests/lock_nonshared.out new file mode 100644 index 0000000000..6b0aea8c04 --- /dev/null +++ b/vlib/v/checker/tests/lock_nonshared.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/lock_nonshared.v:10:7: error: `a` must be declared `shared` to be locked + 8 | x: 5 + 9 | } + 10 | lock a { + | ^ + 11 | a.x++ + 12 | } diff --git a/vlib/v/checker/tests/lock_nonshared.vv b/vlib/v/checker/tests/lock_nonshared.vv new file mode 100644 index 0000000000..1524285ca5 --- /dev/null +++ b/vlib/v/checker/tests/lock_nonshared.vv @@ -0,0 +1,14 @@ +struct St { +mut: + x int +} + +fn main() { + mut a := &St{ + x: 5 + } + lock a { + a.x++ + } + println(a) +} diff --git a/vlib/v/gen/cgen.v b/vlib/v/gen/cgen.v index e64fc5a947..c26cedfe04 100644 --- a/vlib/v/gen/cgen.v +++ b/vlib/v/gen/cgen.v @@ -1892,6 +1892,9 @@ fn (mut g Gen) infix_expr(node ast.InfixExpr) { // } // string + string, string == string etc // g.infix_op = node.op + if node.auto_locked != '' { + g.writeln('sync__RwMutex_w_lock($node.auto_locked->mtx);') + } left_type := g.unwrap_generic(node.left_type) left_sym := g.table.get_type_symbol(left_type) unaliased_left := if left_sym.kind == .alias { (left_sym.info as table.Alias).parent_type } else { left_type } @@ -2121,6 +2124,10 @@ fn (mut g Gen) infix_expr(node ast.InfixExpr) { } } } + if node.auto_locked != '' { + g.writeln(';') + g.write('sync__RwMutex_w_unlock($node.auto_locked->mtx)') + } } fn (mut g Gen) lock_expr(node ast.LockExpr) { diff --git a/vlib/v/tests/autolock_array_1.v b/vlib/v/tests/autolock_array_1.v new file mode 100644 index 0000000000..9919ca40d9 --- /dev/null +++ b/vlib/v/tests/autolock_array_1.v @@ -0,0 +1,52 @@ +import sync +import time + +const ( + iterations_per_thread = 100000 +) + +fn add_elements(shared foo []int, n int) { + for _ in 0 .. iterations_per_thread { + foo << n + } + // automatic lock is not yet implemented for this... + lock foo { + foo[0]++ + } +} + +fn test_autolocked_array() { + shared abc := &[0] + go add_elements(shared abc, 1) + go add_elements(shared abc, 3) + for _ in 0 .. iterations_per_thread { + abc << 0 + } + // wait for coroutines to finish - that should really be + // done by channels, yield, semaphore... + for { + mut finished_threads := 0 + rlock abc { + finished_threads = abc[0] + } + if finished_threads == 2 { + break + } + time.sleep_ms(100) + } + // create histogram of results + mut result := [0, 0, 0, 0] + rlock abc { + // automatic rlock for iteration is also not implemented, yet + for v in abc { + if v > 3 { + panic('unexpected element on array') + } + result[v]++ + } + } + assert result[0] == iterations_per_thread + assert result[1] == iterations_per_thread + assert result[2] == 1 // number of non-main threads + assert result[3] == iterations_per_thread +}