diff --git a/src/check_expr.cpp b/src/check_expr.cpp index aa62a6114..1a4229475 100644 --- a/src/check_expr.cpp +++ b/src/check_expr.cpp @@ -366,6 +366,9 @@ gb_internal void check_scope_decls(CheckerContext *c, Slice const &nodes, check_collect_entities(c, nodes); + // NOTE: checking a declaration can insert entities into this very scope - a procedure alias + // goes through `override_entity_in_scope`, which inserts into the scope being iterated - so + // the iteration must not be invalidated by `s->elements` growing; see ScopeMapIterator. for (auto const &entry : s->elements) { Entity *e = entry.value;\ switch (e->kind) { diff --git a/src/checker.hpp b/src/checker.hpp index 89b6318d7..81bcc8e25 100644 --- a/src/checker.hpp +++ b/src/checker.hpp @@ -465,17 +465,27 @@ gb_internal void scope_map_clear(ScopeMap *m) { m->count = 0; } +// IMPORTANT NOTE: an iteration snapshots the table pointer and capacity when it is constructed, +// so that inserting into the map while iterating it cannot invalidate the iteration. +// `scope_map_insert` may grow the map - rehashing into a newly allocated table - at any time, and +// `begin`/`end` are both created before the loop body runs: without the snapshot, `end` would +// keep the capacity that `begin` saw while `operator++`/`operator*` read the reallocated table, +// walking past the end of it. An iteration therefore walks the table it began on: a slot written +// in place (an insert that does not rehash) is seen when the walk reaches it, while the entries +// written into a table that a rehash replaced are not. struct ScopeMapIterator { ScopeMap const *map; - u32 index; + ScopeMapSlot * slots; + u32 cap; + u32 index; ScopeMapIterator &operator++() noexcept { for (;;) { ++index; - if (map->cap == index) { + if (cap == index) { return *this; } - ScopeMapSlot *s = map->slots+index; + ScopeMapSlot *s = slots+index; if (s->hash) { return *this; } @@ -483,20 +493,20 @@ struct ScopeMapIterator { } bool operator==(ScopeMapIterator const &other) const noexcept { - return this->map == other.map && this->index == other.index; + return this->map == other.map && this->slots == other.slots && this->index == other.index; } operator ScopeMapSlot *() const { - return map->slots+index; + return slots+index; } }; gb_internal ScopeMapIterator end(ScopeMap &m) noexcept { - return ScopeMapIterator{&m, m.cap}; + return ScopeMapIterator{&m, m.slots, m.cap, m.cap}; } gb_internal ScopeMapIterator const end(ScopeMap const &m) noexcept { - return ScopeMapIterator{&m, m.cap}; + return ScopeMapIterator{&m, m.slots, m.cap, m.cap}; } gb_internal ScopeMapIterator begin(ScopeMap &m) noexcept { if (m.count == 0) { @@ -510,7 +520,7 @@ gb_internal ScopeMapIterator begin(ScopeMap &m) noexcept { } index++; } - return ScopeMapIterator{&m, index}; + return ScopeMapIterator{&m, m.slots, m.cap, index}; } gb_internal ScopeMapIterator const begin(ScopeMap const &m) noexcept { if (m.count == 0) { @@ -524,7 +534,7 @@ gb_internal ScopeMapIterator const begin(ScopeMap const &m) noexcept { } index++; } - return ScopeMapIterator{&m, index}; + return ScopeMapIterator{&m, m.slots, m.cap, index}; } enum ScopeFlag : i32 { diff --git a/tests/issues/run.sh b/tests/issues/run.sh index 565528ed8..7cd74fcda 100755 --- a/tests/issues/run.sh +++ b/tests/issues/run.sh @@ -155,6 +155,15 @@ else exit 1 fi +$ODIN test ../test_issue_7598.odin $COMMON + +if [[ $($ODIN build ../test_issue_7598_all_entities_checked.odin $COMMON 2>&1 >/dev/null | grep -c "Error:") -eq 4 ]]; then + echo "SUCCESSFUL 1/1" +else + echo "SUCCESSFUL 0/1" + exit 1 +fi + clang -c ../test_issue_7010.c -o test_issue_7010_c.o $ODIN test ../test_issue_7010.odin $COMMON diff --git a/tests/issues/test_issue_7598.odin b/tests/issues/test_issue_7598.odin new file mode 100644 index 000000000..a57c9ae37 --- /dev/null +++ b/tests/issues/test_issue_7598.odin @@ -0,0 +1,307 @@ +// Tests issue #7598: the compiler crashed with a null pointer dereference when a procedure +// scope held exactly 12 (and, at the next load factor, exactly 24) entities. +// https://github.com/odin-lang/Odin/issues/7598 +// +// `check_scope_decls` iterates the scope's entity map while `check_entity_decl` can insert an +// entity into that very scope: a procedure alias such as `f :: e` goes through +// `override_entity_in_scope`, which inserts into the scope being iterated. The scope map is +// grown at 75% load (12 of 16 slots, then 24 of 32), so the insert rehashes and reallocates the +// table *inside* the loop. The range-for's end sentinel had captured the old capacity, so the +// iteration kept walking the new table past the slot that sentinel named, off the end of the +// table, dereferencing whatever followed it as an `Entity *`. +// +// Both sides of the count are exact, which is what made one declaration more or less compile: +// one entity fewer and the alias insert does not reach the load factor, one entity more and the +// map has already grown while collecting declarations, before the loop starts. +package test_issues + +import "core:testing" + + +// The reported program: 12 entities in one procedure scope, three of them procedure aliases. +// The scope map sits at 75% load (12/16), so the alias insert inside the entity loop rehashes it. +issue_repro :: proc() -> int { + n :: 11 + a :: proc() -> int { return 1 } + b :: proc() -> int { return 2 } + C :: bit_set[1..=9] + d :: proc() -> int { return 4 } + e :: proc() -> int { return 5 } + f :: e + g :: e + h :: e + i :: proc() -> int { return 9 } + j :: proc() -> int { return 10 } + k :: proc() -> int { return 11 } + + std :: size_of(C) + return n + a() + b() + std + d() + e() + f() + g() + h() + i() + j() + k() +} + +// 11 entities: one below the 12/16 load boundary, the map never grows inside the loop. +boundary_11 :: proc() -> int { + n :: 11 + b11_1 :: proc() -> int { return 1 } + b11_2 :: b11_1 + b11_3 :: b11_1 + b11_4 :: proc() -> int { return 4 } + b11_5 :: proc() -> int { return 5 } + b11_6 :: proc() -> int { return 6 } + b11_7 :: proc() -> int { return 7 } + b11_8 :: proc() -> int { return 8 } + b11_9 :: proc() -> int { return 9 } + b11_10 :: proc() -> int { return 10 } + + return n + b11_1() + b11_2() + b11_3() + b11_4() + b11_5() + b11_6() + b11_7() + b11_8() + b11_9() + b11_10() +} + +// 12 entities again, with names that rehash into different slots: a second layout at the boundary. +boundary_12_alt :: proc() -> int { + n :: 11 + c12_1 :: proc() -> int { return 1 } + c12_2 :: proc() -> int { return 2 } + c12_3 :: proc() -> int { return 3 } + c12_4 :: c12_1 + c12_5 :: c12_1 + c12_6 :: proc() -> int { return 6 } + c12_7 :: proc() -> int { return 7 } + c12_8 :: proc() -> int { return 8 } + c12_9 :: proc() -> int { return 9 } + c12_10 :: proc() -> int { return 10 } + c12_11 :: proc() -> int { return 11 } + + return n + c12_1() + c12_2() + c12_3() + c12_4() + c12_5() + c12_6() + c12_7() + c12_8() + c12_9() + c12_10() + c12_11() +} + +// 12 entities, third layout, with the aliases visited later in the iteration. +boundary_12_alt2 :: proc() -> int { + n :: 11 + d12_1 :: proc() -> int { return 1 } + d12_2 :: proc() -> int { return 2 } + d12_3 :: proc() -> int { return 3 } + d12_4 :: proc() -> int { return 4 } + d12_5 :: proc() -> int { return 5 } + d12_6 :: d12_1 + d12_7 :: d12_1 + d12_8 :: d12_1 + d12_9 :: proc() -> int { return 9 } + d12_10 :: proc() -> int { return 10 } + d12_11 :: proc() -> int { return 11 } + + return n + d12_1() + d12_2() + d12_3() + d12_4() + d12_5() + d12_6() + d12_7() + d12_8() + d12_9() + d12_10() + d12_11() +} + +// 13 entities: the map has already grown while collecting, so it does not grow inside the loop. +boundary_13 :: proc() -> int { + n :: 11 + e13_1 :: proc() -> int { return 1 } + e13_2 :: e13_1 + e13_3 :: e13_1 + e13_4 :: proc() -> int { return 4 } + e13_5 :: proc() -> int { return 5 } + e13_6 :: proc() -> int { return 6 } + e13_7 :: proc() -> int { return 7 } + e13_8 :: proc() -> int { return 8 } + e13_9 :: proc() -> int { return 9 } + e13_10 :: proc() -> int { return 10 } + e13_11 :: proc() -> int { return 11 } + e13_12 :: proc() -> int { return 12 } + + return n + e13_1() + e13_2() + e13_3() + e13_4() + e13_5() + e13_6() + e13_7() + e13_8() + e13_9() + e13_10() + e13_11() + e13_12() +} + +// 23 entities: the second grow point (24/32) is not reached. +boundary_23 :: proc() -> int { + n :: 11 + f23_1 :: proc() -> int { return 1 } + f23_2 :: f23_1 + f23_3 :: f23_1 + f23_4 :: proc() -> int { return 4 } + f23_5 :: proc() -> int { return 5 } + f23_6 :: proc() -> int { return 6 } + f23_7 :: proc() -> int { return 7 } + f23_8 :: proc() -> int { return 8 } + f23_9 :: proc() -> int { return 9 } + f23_10 :: proc() -> int { return 10 } + f23_11 :: proc() -> int { return 11 } + f23_12 :: proc() -> int { return 12 } + f23_13 :: proc() -> int { return 13 } + f23_14 :: proc() -> int { return 14 } + f23_15 :: proc() -> int { return 15 } + f23_16 :: proc() -> int { return 16 } + f23_17 :: proc() -> int { return 17 } + f23_18 :: proc() -> int { return 18 } + f23_19 :: proc() -> int { return 19 } + f23_20 :: proc() -> int { return 20 } + f23_21 :: proc() -> int { return 21 } + f23_22 :: proc() -> int { return 22 } + + return n + f23_1() + f23_2() + f23_3() + f23_4() + f23_5() + f23_6() + f23_7() + f23_8() + f23_9() + f23_10() + f23_11() + f23_12() + f23_13() + f23_14() + f23_15() + f23_16() + f23_17() + f23_18() + f23_19() + f23_20() + f23_21() + f23_22() +} + +// 24 entities: the scope map is at 75% load again (24/32) and grows 32 -> 64 inside the loop. +boundary_24 :: proc() -> int { + n :: 11 + g24_1 :: proc() -> int { return 1 } + g24_2 :: g24_1 + g24_3 :: g24_1 + g24_4 :: proc() -> int { return 4 } + g24_5 :: proc() -> int { return 5 } + g24_6 :: proc() -> int { return 6 } + g24_7 :: proc() -> int { return 7 } + g24_8 :: proc() -> int { return 8 } + g24_9 :: proc() -> int { return 9 } + g24_10 :: proc() -> int { return 10 } + g24_11 :: proc() -> int { return 11 } + g24_12 :: proc() -> int { return 12 } + g24_13 :: proc() -> int { return 13 } + g24_14 :: proc() -> int { return 14 } + g24_15 :: proc() -> int { return 15 } + g24_16 :: proc() -> int { return 16 } + g24_17 :: proc() -> int { return 17 } + g24_18 :: proc() -> int { return 18 } + g24_19 :: proc() -> int { return 19 } + g24_20 :: proc() -> int { return 20 } + g24_21 :: proc() -> int { return 21 } + g24_22 :: proc() -> int { return 22 } + g24_23 :: proc() -> int { return 23 } + + return n + g24_1() + g24_2() + g24_3() + g24_4() + g24_5() + g24_6() + g24_7() + g24_8() + g24_9() + g24_10() + g24_11() + g24_12() + g24_13() + g24_14() + g24_15() + g24_16() + g24_17() + g24_18() + g24_19() + g24_20() + g24_21() + g24_22() + g24_23() +} + +// 24 entities, second layout at the second grow point. +boundary_24_alt :: proc() -> int { + n :: 11 + h24_1 :: proc() -> int { return 1 } + h24_2 :: proc() -> int { return 2 } + h24_3 :: proc() -> int { return 3 } + h24_4 :: proc() -> int { return 4 } + h24_5 :: proc() -> int { return 5 } + h24_6 :: proc() -> int { return 6 } + h24_7 :: h24_1 + h24_8 :: h24_1 + h24_9 :: proc() -> int { return 9 } + h24_10 :: proc() -> int { return 10 } + h24_11 :: proc() -> int { return 11 } + h24_12 :: proc() -> int { return 12 } + h24_13 :: proc() -> int { return 13 } + h24_14 :: proc() -> int { return 14 } + h24_15 :: proc() -> int { return 15 } + h24_16 :: proc() -> int { return 16 } + h24_17 :: proc() -> int { return 17 } + h24_18 :: proc() -> int { return 18 } + h24_19 :: proc() -> int { return 19 } + h24_20 :: proc() -> int { return 20 } + h24_21 :: proc() -> int { return 21 } + h24_22 :: proc() -> int { return 22 } + h24_23 :: proc() -> int { return 23 } + + return n + h24_1() + h24_2() + h24_3() + h24_4() + h24_5() + h24_6() + h24_7() + h24_8() + h24_9() + h24_10() + h24_11() + h24_12() + h24_13() + h24_14() + h24_15() + h24_16() + h24_17() + h24_18() + h24_19() + h24_20() + h24_21() + h24_22() + h24_23() +} + +// 25 entities: the map grew while collecting, so it does not grow inside the loop. +boundary_25 :: proc() -> int { + n :: 11 + i25_1 :: proc() -> int { return 1 } + i25_2 :: i25_1 + i25_3 :: i25_1 + i25_4 :: proc() -> int { return 4 } + i25_5 :: proc() -> int { return 5 } + i25_6 :: proc() -> int { return 6 } + i25_7 :: proc() -> int { return 7 } + i25_8 :: proc() -> int { return 8 } + i25_9 :: proc() -> int { return 9 } + i25_10 :: proc() -> int { return 10 } + i25_11 :: proc() -> int { return 11 } + i25_12 :: proc() -> int { return 12 } + i25_13 :: proc() -> int { return 13 } + i25_14 :: proc() -> int { return 14 } + i25_15 :: proc() -> int { return 15 } + i25_16 :: proc() -> int { return 16 } + i25_17 :: proc() -> int { return 17 } + i25_18 :: proc() -> int { return 18 } + i25_19 :: proc() -> int { return 19 } + i25_20 :: proc() -> int { return 20 } + i25_21 :: proc() -> int { return 21 } + i25_22 :: proc() -> int { return 22 } + i25_23 :: proc() -> int { return 23 } + i25_24 :: proc() -> int { return 24 } + + return n + i25_1() + i25_2() + i25_3() + i25_4() + i25_5() + i25_6() + i25_7() + i25_8() + i25_9() + i25_10() + i25_11() + i25_12() + i25_13() + i25_14() + i25_15() + i25_16() + i25_17() + i25_18() + i25_19() + i25_20() + i25_21() + i25_22() + i25_23() + i25_24() +} + +// The same boundary in a nested block scope: the declaration-checking flag reaches nested blocks +// as well, so the block's own scope is collected and iterated exactly like a procedure body's. +boundary_12_block :: proc() -> int { + total := 0 + { + n :: 11 + blk_1 :: proc() -> int { return 1 } + blk_2 :: blk_1 + blk_3 :: blk_1 + blk_4 :: proc() -> int { return 4 } + blk_5 :: proc() -> int { return 5 } + blk_6 :: proc() -> int { return 6 } + blk_7 :: proc() -> int { return 7 } + blk_8 :: proc() -> int { return 8 } + blk_9 :: proc() -> int { return 9 } + blk_10 :: proc() -> int { return 10 } + blk_11 :: proc() -> int { return 11 } + + total = n + blk_1() + blk_2() + blk_3() + blk_4() + blk_5() + blk_6() + blk_7() + blk_8() + blk_9() + blk_10() + blk_11() + } + return total +} + +// Every case is called and its result checked: an entity whose declaration the entity loop +// dropped has to fail here, and a compiler that cannot check the scope has to fail before it +// gets this far. +@(test) +test_issue_7598_issue_repro :: proc(t: ^testing.T) { + testing.expect_value(t, issue_repro(), 70) +} + +@(test) +test_issue_7598_boundary_11 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_11(), 63) +} + +@(test) +test_issue_7598_boundary_12_alt :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_12_alt(), 70) +} + +@(test) +test_issue_7598_boundary_12_alt2 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_12_alt2(), 59) +} + +@(test) +test_issue_7598_boundary_12_block :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_12_block(), 74) +} + +@(test) +test_issue_7598_boundary_13 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_13(), 86) +} + +@(test) +test_issue_7598_boundary_23 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_23(), 261) +} + +@(test) +test_issue_7598_boundary_24 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_24(), 284) +} + +@(test) +test_issue_7598_boundary_24_alt :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_24_alt(), 274) +} + +@(test) +test_issue_7598_boundary_25 :: proc(t: ^testing.T) { + testing.expect_value(t, boundary_25(), 308) +} \ No newline at end of file diff --git a/tests/issues/test_issue_7598_all_entities_checked.odin b/tests/issues/test_issue_7598_all_entities_checked.odin new file mode 100644 index 000000000..3b043b76f --- /dev/null +++ b/tests/issues/test_issue_7598_all_entities_checked.odin @@ -0,0 +1,55 @@ +// Companion to test_issue_7598.odin, covering the other half of issue #7598. +// https://github.com/odin-lang/Odin/issues/7598 +// +// When the scope map is rehashed from inside `check_scope_decls`' iteration, the iteration +// either runs off the end of the reallocated table (the crash in test_issue_7598.odin) or it +// stops at the slot the stale end sentinel named and silently drops every entity whose new slot +// lies behind it. A dropped entity is never checked, so an invalid declaration simply produces +// no diagnostic at all. +// +// Every invalid declaration here must be reported: 2 declarations x 2 procedure scopes = 4 +// errors. `boundary_errors_13` holds 13 entities, one above the 12/16 load boundary, so its map +// has already grown by the time the loop starts and it reports its own 2 errors even without the +// fix; `boundary_errors_12` holds exactly 12 entities and its map grows *inside* the loop, so +// without the fix it swallows one of its 2. +package test_issues + +boundary_errors_13 :: proc() -> int { + n :: 11 + a0 :: proc() -> int { return 1 } + b1 :: a0 + c2 :: a0 + d3 :: proc() -> int { return 4 } + e4 :: undeclared_name_5 + f5 :: proc() -> int { return 6 } + g6 :: proc() -> int { return 7 } + h7 :: undeclared_name_8 + i8 :: proc() -> int { return 9 } + j9 :: proc() -> int { return 10 } + k10 :: proc() -> int { return 11 } + l11 :: proc() -> int { return 12 } + + return n + a0() + b1() + c2() + d3() + f5() + g6() + i8() + j9() + k10() + l11() +} + +boundary_errors_12 :: proc() -> int { + n :: 11 + a0 :: proc() -> int { return 1 } + b1 :: a0 + c2 :: a0 + d3 :: proc() -> int { return 4 } + e4 :: undeclared_name_5 + f5 :: proc() -> int { return 6 } + g6 :: proc() -> int { return 7 } + h7 :: undeclared_name_8 + i8 :: proc() -> int { return 9 } + j9 :: proc() -> int { return 10 } + k10 :: proc() -> int { return 11 } + + return n + a0() + b1() + c2() + d3() + f5() + g6() + i8() + j9() + k10() +} + +main :: proc() { + _ = boundary_errors_13() + _ = boundary_errors_12() +} \ No newline at end of file