all: make `lock` and `rlock` dead lock free :-) (#8534)

vlib/v/ast/ast.v

@@ -646,7 +646,7 @@ pub:
 pub struct LockExpr {
 pub:
 	stmts    []Stmt
-	is_rlock bool
+	is_rlock []bool
 	pos      token.Position
 pub mut:
 	lockeds []Ident // `x`, `y` in `lock x, y {`

vlib/v/checker/checker.v

@@ -1081,6 +1081,15 @@ fn (mut c Checker) fail_if_immutable(expr ast.Expr) (string, token.Position) {
 				v.is_changed = true
 				if v.typ.share() == .shared_t {
 					if expr.name !in c.locked_names {
+						if c.locked_names.len > 0 || c.rlocked_names.len > 0 {
+							if expr.name in c.rlocked_names {
+								c.error('$expr.name has an `rlock` but needs a `lock`',
+									expr.pos)
+							} else {
+								c.error('$expr.name must be added to the `lock` list above',
+									expr.pos)
+							}
+						}
 						to_lock = expr.name
 						pos = expr.pos
 					}
@@ -2620,6 +2629,8 @@ pub fn (mut c Checker) assign_stmt(mut assign_stmt ast.AssignStmt) {
 							}
 							left_type = left_type.set_nr_muls(1)
 						}
+					} else if left_type.has_flag(.shared_f) {
+						left_type = left_type.clear_flag(.shared_f)
 					}
 					if ident_var_info.share == .atomic_t {
 						left_type = left_type.set_flag(.atomic_f)
@@ -4540,6 +4551,9 @@ pub fn (mut c Checker) select_expr(mut node ast.SelectExpr) table.Type {
 }
 
 pub fn (mut c Checker) lock_expr(mut node ast.LockExpr) table.Type {
+	if c.rlocked_names.len > 0 || c.locked_names.len > 0 {
+		c.error('nested `lock`/`rlock` not allowed', node.pos)
+	}
 	for i in 0 .. node.lockeds.len {
 		c.ident(mut node.lockeds[i])
 		id := node.lockeds[i]
@@ -4555,18 +4569,15 @@ pub fn (mut c Checker) lock_expr(mut node ast.LockExpr) table.Type {
 		} else if id.name in c.rlocked_names {
 			c.error('`$id.name` is already read-locked', id.pos)
 		}
-		if node.is_rlock {
+		if node.is_rlock[i] {
 			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]
-	}
+	c.rlocked_names = []
+	c.locked_names = []
 	// void for now... maybe sometime `x := lock a { a.getval() }`
 	return table.void_type
 }

vlib/v/checker/tests/lock_already_locked.out

@@ -1,7 +1,7 @@
-vlib/v/checker/tests/lock_already_locked.vv:11:9: error: `a` is already locked 
+vlib/v/checker/tests/lock_already_locked.vv:11:3: error: nested `lock`/`rlock` not allowed
     9 |     }
    10 |     lock a {
    11 |         rlock a {
-      |               ^
+      |         ~~~~~
    12 |             a.x++
    13 |         }

vlib/v/checker/tests/lock_already_rlocked.out

@@ -1,7 +1,7 @@
-vlib/v/checker/tests/lock_already_rlocked.vv:11:8: error: `a` is already read-locked 
+vlib/v/checker/tests/lock_already_rlocked.vv:11:3: error: nested `lock`/`rlock` not allowed
     9 |     }
    10 |     rlock a {
    11 |         lock a {
-      |              ^
+      |         ~~~~
    12 |             a.x++
    13 |         }

vlib/v/checker/tests/shared_bad_args.out

@@ -26,14 +26,14 @@ vlib/v/checker/tests/shared_bad_args.vv:51:13: error: a is `shared` and must be
       |                ^
    52 |     println('$w $x')
    53 | }
-vlib/v/checker/tests/shared_bad_args.vv:61:3: error: r is `shared` and must be `lock`ed to be passed as `mut`
+vlib/v/checker/tests/shared_bad_args.vv:61:3: error: r must be added to the `lock` list above
    59 |     shared r := Qr{ a: 7 }
    60 |     lock s {
    61 |         r.s_mut(mut s)
       |         ^
    62 |     }
    63 |     lock r {
-vlib/v/checker/tests/shared_bad_args.vv:64:15: error: s is `shared` and must be `lock`ed to be passed as `mut`
+vlib/v/checker/tests/shared_bad_args.vv:64:15: error: s must be added to the `lock` list above
    62 |     }
    63 |     lock r {
    64 |         r.s_mut(mut s)

vlib/v/fmt/fmt.v

@@ -1480,12 +1480,43 @@ pub fn (mut f Fmt) array_decompose(node ast.ArrayDecompose) {
 }
 
 pub fn (mut f Fmt) lock_expr(lex ast.LockExpr) {
-	f.write(if lex.is_rlock { 'rlock ' } else { 'lock ' })
+	mut num_locked := 0
+	mut num_rlocked := 0
+	for is_rlock in lex.is_rlock {
+		if is_rlock {
+			num_rlocked++
+		} else {
+			num_locked++
+		}
+	}
+	if num_locked > 0 || num_rlocked == 0 {
+		f.write('lock ')
+		mut n := 0
 		for i, v in lex.lockeds {
-		if i > 0 {
+			if !lex.is_rlock[i] {
+				if n > 0 {
 					f.write(', ')
 				}
 				f.expr(v)
+				n++
+			}
+		}
+	}
+	if num_rlocked > 0 {
+		if num_locked > 0 {
+			f.write('; ')
+		}
+		f.write('rlock ')
+		mut n := 0
+		for i, v in lex.lockeds {
+			if lex.is_rlock[i] {
+				if n > 0 {
+					f.write(', ')
+				}
+				f.expr(v)
+				n++
+			}
+		}
 	}
 	f.write(' {')
 	f.writeln('')

vlib/v/gen/c/cgen.v

@@ -3405,22 +3405,54 @@ fn (mut g Gen) infix_expr(node ast.InfixExpr) {
 }
 
 fn (mut g Gen) lock_expr(node ast.LockExpr) {
-	mut lock_prefixes := []string{len: 0, cap: node.lockeds.len}
-	for id in node.lockeds {
+	mut mtxs := ''
+	if node.lockeds.len == 0 {
+		// this should not happen
+	} else if node.lockeds.len == 1 {
+		id := node.lockeds[0]
 		name := id.name
 		deref := if id.is_mut { '->' } else { '.' }
-		lock_prefix := if node.is_rlock { 'r' } else { '' }
-		lock_prefixes << lock_prefix // keep for unlock
+		lock_prefix := if node.is_rlock[0] { 'r' } else { '' }
 		g.writeln('sync__RwMutex_${lock_prefix}lock(&$name${deref}mtx);')
+	} else {
+		mtxs = g.new_tmp_var()
+		g.writeln('uintptr_t _arr_$mtxs[$node.lockeds.len];')
+		g.writeln('bool _isrlck_$mtxs[$node.lockeds.len];')
+		for i, id in node.lockeds {
+			name := id.name
+			deref := if id.is_mut { '->' } else { '.' }
+			g.writeln('_arr_$mtxs[$i] = &$name${deref}mtx;')
+			// TODO: fix `vfmt` to allow this in string interpolation
+			is_rlock_str := node.is_rlock[i].str()
+			g.writeln('_isrlck_$mtxs[$i] = $is_rlock_str;')
+		}
+		g.writeln('__sort_ptr(_arr_$mtxs, _isrlck_$mtxs, $node.lockeds.len);')
+		g.writeln('for (int $mtxs=0; $mtxs<$node.lockeds.len; $mtxs++) {')
+		g.writeln('\tif ($mtxs && _arr_$mtxs[$mtxs] == _arr_$mtxs[$mtxs-1]) continue;')
+		g.writeln('\tif (_isrlck_$mtxs[$mtxs])')
+		g.writeln('\t\tsync__RwMutex_rlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);')
+		g.writeln('\telse')
+		g.writeln('\t\tsync__RwMutex_lock((sync__RwMutex*)_arr_$mtxs[$mtxs]);')
+		g.writeln('}')
 	}
 	g.stmts(node.stmts)
-	// unlock in reverse order
-	for i := node.lockeds.len - 1; i >= 0; i-- {
-		id := node.lockeds[i]
-		lock_prefix := lock_prefixes[i]
+	if node.lockeds.len == 0 {
+		// this should not happen
+	} else if node.lockeds.len == 1 {
+		id := node.lockeds[0]
 		name := id.name
 		deref := if id.is_mut { '->' } else { '.' }
+		lock_prefix := if node.is_rlock[0] { 'r' } else { '' }
 		g.writeln('sync__RwMutex_${lock_prefix}unlock(&$name${deref}mtx);')
+	} else {
+		// unlock in reverse order
+		g.writeln('for (int $mtxs=${node.lockeds.len - 1}; $mtxs>=0; $mtxs--) {')
+		g.writeln('\tif ($mtxs && _arr_$mtxs[$mtxs] == _arr_$mtxs[$mtxs-1]) continue;')
+		g.writeln('\tif (_isrlck_$mtxs[$mtxs])')
+		g.writeln('\t\tsync__RwMutex_runlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);')
+		g.writeln('\telse')
+		g.writeln('\t\tsync__RwMutex_unlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);')
+		g.writeln('}')
 	}
 }
 

vlib/v/gen/c/cheaders.v

@@ -31,6 +31,21 @@ static inline voidptr __dup_shared_array(voidptr src, int sz) {
 	sync__RwMutex_init(&dest->mtx);
 	return dest;
 }
+static inline void __sort_ptr(uintptr_t a[], bool b[], int l)
+{
+	for (int i=1; i<l; i++) {
+		uintptr_t ins = a[i];
+		bool insb = b[i];
+		int j = i;
+		while(j>0 && (a[j-1] > ins || b[j-1] && !insb)) {
+			a[j] = a[j-1];
+			b[j] = b[j-1];
+			j--;
+		}
+		a[j] = ins;
+		b[j] = insb;
+	}
+}
 '
 	c_common_macros               = '
 #define EMPTY_VARG_INITIALIZATION 0

vlib/v/parser/lock.v

@@ -7,9 +7,17 @@ fn (mut p Parser) lock_expr() ast.LockExpr {
 	// TODO Handle aliasing sync
 	p.register_auto_import('sync')
 	mut pos := p.tok.position()
-	is_rlock := p.tok.kind == .key_rlock
-	p.next()
 	mut lockeds := []ast.Ident{}
+	mut is_rlocked := []bool{}
+	for {
+		if p.tok.kind == .lcbr {
+			goto start_stmts
+		}
+		is_rlock := p.tok.kind == .key_rlock
+		if !is_rlock && p.tok.kind != .key_lock {
+			p.error_with_pos('unexpected `$p.tok.lit`; `lock` or `rlock` expected', p.tok.position())
+		}
+		p.next()
 		for p.tok.kind == .name {
 			lockeds << ast.Ident{
 				language: table.Language.v
@@ -20,18 +28,25 @@ fn (mut p Parser) lock_expr() ast.LockExpr {
 				info: ast.IdentVar{}
 				scope: p.scope
 			}
+			is_rlocked << is_rlock
 			p.next()
 			if p.tok.kind == .lcbr {
+				goto start_stmts
+			}
+			if p.tok.kind == .semicolon {
+				p.next()
 				break
 			}
 			p.check(.comma)
 		}
+	}
+	start_stmts:
 	stmts := p.parse_block()
 	pos.update_last_line(p.prev_tok.line_nr)
 	return ast.LockExpr{
 		lockeds: lockeds
 		stmts: stmts
-		is_rlock: is_rlock
+		is_rlock: is_rlocked
 		pos: pos
 	}
 }

vlib/v/tests/shared_lock_5_test.v

@@ -0,0 +1,68 @@
+struct St {
+mut:
+	a int
+}
+
+fn (shared x St) f(shared y St, shared z St) {
+	for _ in 0 .. 10000 {
+		lock x, y, z {
+			tmp := z.a
+			z.a = y.a
+			y.a = x.a
+			x.a = tmp
+		}
+	}
+}
+
+fn (shared x St) g(shared y St, shared z St) {
+	for _ in 0 .. 10000 {
+		lock z, x, y {
+			tmp := x.a
+			x.a = z.a
+			z.a = y.a
+			y.a = tmp
+		}
+	}
+}
+
+fn h(shared x St, shared y St, shared z St) {
+	for _ in 0 .. 10000 {
+		lock y, x, z {
+			tmp := y.a
+			y.a = z.a
+			z.a = x.a
+			x.a = tmp
+		}
+	}
+}
+
+fn test_shared_receiver_lock() {
+	shared x := &St{
+		a: 5
+	}
+	shared y := &St{
+		a: 7
+	}
+	shared z := &St{
+		a: 1
+	}
+	t1 := go x.f(shared y, shared z)
+	t2 := go x.f(shared y, shared z)
+	t3 := go h(shared x, shared y, shared z)
+	for _ in 0 .. 10000 {
+		lock z, y, x {
+			tmp := y.a
+			y.a = x.a
+			x.a = z.a
+			z.a = tmp
+		}
+	}
+	t1.wait()
+	t2.wait()
+	t3.wait()
+	rlock x, y, z{
+		assert x.a == 7
+		assert y.a == 1
+		assert z.a == 5
+	}
+}

vlib/v/tests/shared_lock_6_test.v

@@ -0,0 +1,59 @@
+struct St {
+mut:
+	a int
+}
+
+fn (shared x St) f(shared y St, shared z St) {
+	for _ in 0 .. 10000 {
+		lock x; rlock y, z {
+			x.a = y.a + z.a
+			if x.a > 1000000 {
+				x.a /= 2
+			}
+		}
+	}
+}
+
+fn (shared x St) g(shared y St, shared z St) {
+	for _ in 0 .. 10000 {
+		rlock x; lock y, z {
+			y.a += x.a
+			if y.a > 1000000 {
+				y.a /= 2
+			}
+			z.a += x.a
+			if z.a > 1000000 {
+				z.a /= 2
+			}
+		}
+	}
+}
+
+fn test_shared_receiver_lock() {
+	shared x := St{
+		a: 5
+	}
+	shared y := St{
+		a: 7
+	}
+	shared z := St{
+		a: 103
+	}
+	t1 := go x.f(shared y, shared x)
+	t2 := go y.f(shared y, shared z)
+	t3 := go z.g(shared x, shared z)
+	t4 := go x.g(shared x, shared x)
+	t5 := go z.f(shared z, shared z)
+	t1.wait()
+	t2.wait()
+	t3.wait()
+	t4.wait()
+	t5.wait()
+	// the result is unpredictable - but should be positive
+	rlock x, y, z {
+		assert x.a > 10000
+		assert y.a > 10000
+		assert z.a > 10000
+		println('$x.a $y.a $z.a')
+	}
+}

vlib/v/tests/shared_unordered_mixed_test.v

@@ -0,0 +1,66 @@
+// integer values from -2^191 .. 2^191-1
+struct Large {
+mut:
+	l u64
+	m u64
+	h u64
+}
+
+fn (a Large) clone() Large {
+	r := Large{ l: a.l, m: a.m h: a.h }
+	return r
+}
+
+fn (mut a Large) add(b Large) {
+	oldl := a.l
+	a.l += b.l
+	oldm := a.m
+	if a.l < oldl { a.m++ }
+	a.m += b.m
+	if a.m < oldm || (a.l < oldl && a.m <= oldm) { a.h++ }
+	a.h += b.h
+}
+
+fn doub_large(shared a Large, shared b Large, shared c Large, shared d Large, shared e Large) {
+	for _ in 0..50 {
+		lock a, b; rlock c, d, e {
+			// double the sum by adding all objects to a or b
+			old_a := a.clone()
+			a.add(b)
+			b.add(old_a)
+			a.add(c)
+			b.add(d)
+			a.add(e)
+		}
+	}
+}
+
+fn test_mixed_order_lock_rlock() {
+	// initialze objects so that their sum = 1
+	shared a := Large{ l: 4 }
+	shared b := Large{ l: u64(-7), m: u64(-1) h: u64(-1) }
+	shared c := Large{ l: 17 }
+	shared d := Large{ l: u64(-11), m: u64(-1) h: u64(-1) }
+	shared e := Large{ l: u64(-2), m: u64(-1) h: u64(-1) }
+	// spawn 3 threads for doubling with different orders of objects
+	t1 := go doub_large(shared a, shared b, shared c, shared d, shared e)
+	t2 := go doub_large(shared e, shared b, shared a, shared c, shared d)
+	t3 := go doub_large(shared b, shared a, shared e, shared d, shared c)
+	t1.wait()
+	t2.wait()
+	t3.wait()
+	// calculate the sum after 3*50 doublings
+	mut sum := Large{}
+	rlock a, b, c, d, e {
+		sum.add(a)
+		sum.add(b)
+		sum.add(c)
+		sum.add(d)
+		sum.add(e)
+	}
+	// the sum should be 2^150
+	assert sum.l == 0
+	assert sum.m == 0
+	assert sum.h == 4194304
+}
+