vm+gc: Clean up GC and VM logic

Also added gc python module to control GC

Author: gf712

integration/run_python_tests.sh

@@ -5,7 +5,7 @@
 SCRIPT_DIR="$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
 
 PYTHON_EXECUTABLE=$1
-GC_FREQUENCY=100000
+GC_FREQUENCY=${GC_FREQUENCY:-100000}
 
 # start by calling the tests that we need to work in order to trust the result of the other python tests
 if timeout 10s $PYTHON_EXECUTABLE $SCRIPT_DIR/tests/lemmas/assert_false.py --gc-frequency $GC_FREQUENCY &> /dev/null; then

src/CMakeLists.txt

@@ -144,6 +144,7 @@ set(RUNTIME_SOURCE_FILES
     runtime/modules/struct/module.cpp
     runtime/modules/SysModule.cpp
     runtime/modules/WarningsModule.cpp
+    runtime/modules/GcModule.cpp
     runtime/types/builtin.cpp
     runtime/warnings/DeprecationWarning.cpp
     runtime/warnings/ImportWarning.cpp

src/executable/bytecode/Bytecode.cpp

@@ -106,14 +106,14 @@ PyResult<Value> Bytecode::call_without_setup(VirtualMachine &vm, Interpreter &in
 	// create main stack frame
 	ASSERT(!vm.stack().empty());
 
-	constexpr auto sentinel = decltype(vm.stack().top().get().last_instruction_pointer)();
-	if (vm.stack().top().get().last_instruction_pointer == sentinel) {
+	constexpr auto sentinel = decltype(vm.stack().back().get().last_instruction_pointer)();
+	if (vm.stack().back().get().last_instruction_pointer == sentinel) {
 		// first time calling with the stack frame, so we don't have a last instruction pointer yet
 		vm.set_instruction_pointer(begin());
 	} else {
 		// otherwise resume execution, by starting execution from the instruction after the last run
 		// instruction
-		vm.set_instruction_pointer(vm.stack().top().get().last_instruction_pointer + 1);
+		vm.set_instruction_pointer(vm.stack().back().get().last_instruction_pointer + 1);
 	}
 
 	return eval_loop(vm, interpreter);

src/interpreter/Interpreter.cpp

@@ -269,7 +269,7 @@ PyModule *Interpreter::get_imported_module(PyString *name) const
 ScopedStack::~ScopedStack()
 {
 	auto &vm = VirtualMachine::the();
-	if (!vm.stack().empty() && top_frame && &vm.stack().top().get() == top_frame.get()) {
+	if (!vm.stack().empty() && top_frame && &vm.stack().back().get() == top_frame.get()) {
 		vm.pop_frame(true);
 	}
 }

src/memory/GarbageCollector.cpp

@@ -59,27 +59,38 @@ __attribute__((no_sanitize_address)) std::stack<Cell *> collect_roots_on_the_sta
 {
 	std::stack<Cell *> roots;
 
-	// push register values onto the stack
+	// Spill all callee-saved registers into `jump_buffer`. setjmp captures
+	// them as part of saving the calling environment.
 	std::jmp_buf jump_buffer;
 	setjmp(jump_buffer);
 
-	// traverse the stack from the stack pointer to the stack origin/bottom
-	// uintptr_t *rsp_;
-	// asm volatile("movq %%rsp, %0" : "=r"(rsp_));
+	auto scan_range = [&](uint8_t *begin, uint8_t *end) {
+		for (uint8_t *p = begin; p < end; p += sizeof(uintptr_t)) {
+			uint8_t *address =
+				bit_cast_without_sanitizer<uint8_t *>(*bit_cast_without_sanitizer<uintptr_t *>(p))
+				- sizeof(GarbageCollected);
+			spdlog::trace("checking address {}, pointer address={}", (void *)address, (void *)p);
+			if (heap.slab().has_address(address)) {
+				spdlog::trace("valid address {}", (void *)address);
+				auto *obj_header = bit_cast<GarbageCollected *>(address);
+				add_root(obj_header, roots);
+			}
+		}
+	};
 
-	// uint8_t *rsp = bit_cast<uint8_t *>(rsp_);
+	// jump_buffer is a local variable, so it sits below the current frame
+	// pointer on stacks that grow downward. The frame-pointer-to-stack-bottom
+	// scan below therefore skips it, which would lose any GC root whose only
+	// live reference is in a callee-saved register at this point. Scan the
+	// buffer's bytes explicitly to recover those spilled values.
+	auto *jb_begin = bit_cast_without_sanitizer<uint8_t *>(&jump_buffer);
+	scan_range(jb_begin, jb_begin + sizeof(jump_buffer));
+
+	// Traverse the stack from the current frame pointer up to the recorded
+	// stack bottom; this covers everything in our callers' frames.
 	uint8_t *rsp = bit_cast_without_sanitizer<uint8_t *>(__builtin_frame_address(0));
-	for (; rsp < stack_bottom; rsp += sizeof(uintptr_t)) {
-		uint8_t *address =
-			bit_cast_without_sanitizer<uint8_t *>(*bit_cast_without_sanitizer<uintptr_t *>(rsp))
-			- sizeof(GarbageCollected);
-		spdlog::trace("checking address {}, pointer address={}", (void *)address, (void *)rsp);
-		if (heap.slab().has_address(address)) {
-			spdlog::trace("valid address {}", (void *)address);
-			auto *obj_header = bit_cast<GarbageCollected *>(address);
-			add_root(obj_header, roots);
-		}
-	}
+	scan_range(rsp, stack_bottom);
+
 	spdlog::debug("Done collecting roots from the stack, found {} roots", roots.size());
 	return roots;
 }
@@ -149,20 +160,27 @@ struct MarkGCVisitor : Cell::Visitor
 	Heap &m_heap;
 	std::stack<Cell *> &m_to_visit;
 
+	// Collects the 1-hop neighbours of a given cell. visit_graph(visitor) on
+	// the root invokes Visitor::visit() for each thing the root references
+	// (which, by convention, may include the root itself for leaf cells); the
+	// `m_collecting` gate isolates those nested calls so only direct
+	// neighbours land in the vector and we do not recurse further.
 	struct NeighbourVisitor : Cell::Visitor
 	{
 		std::vector<Cell *> m_neighbours;
-		size_t m_depth{ 0 };
+		bool m_collecting{ false };
 
-		void visit(Cell &cell)
+		void collect_neighbours_of(Cell &root)
 		{
-			m_depth++;
-			if (m_depth == 1) {
-				cell.visit_graph(*this);
-			} else if (m_depth == 2) {
-				m_neighbours.push_back(&cell);
-			}
-			m_depth--;
+			m_neighbours.clear();
+			m_collecting = true;
+			root.visit_graph(*this);
+			m_collecting = false;
+		}
+
+		void visit(Cell &cell) override
+		{
+			if (m_collecting) { m_neighbours.push_back(&cell); }
 		}
 	};
 
@@ -174,7 +192,7 @@ struct MarkGCVisitor : Cell::Visitor
 
 		if (!is_static_memory(cell_start, m_heap)) {
 			NeighbourVisitor nv{};
-			nv.visit(cell);
+			nv.collect_neighbours_of(cell);
 			auto &neighbours = nv.m_neighbours;
 			spdlog::trace("node: {}", static_cast<void *>(&cell));
 			for (auto *neighbour : neighbours) {
@@ -212,8 +230,15 @@ struct MarkGCVisitor : Cell::Visitor
 
 void MarkSweepGC::mark_all_cell_unreachable(Heap &heap) const
 {
-	// TODO: once the ideal block sizes are fixed there should be an iterator
-	//       returning a list of all blocks
+	// NOTE: this site intentionally does NOT use Slab::for_each_block. Doing
+	// so produces a stack layout (lambda captures across inlining) that
+	// keeps a Block pointer alive in a slot the next collect_roots scan
+	// reads as a heap root, regressing
+	// TestHeap.GarbageCollectorDeallocatesGCPointersWhenStackFrameIsPopped.
+	// The conservative-GC fragility this exposes is the underlying issue;
+	// until it is replaced by a precise RootSet, keep the explicit array
+	// here so the test stays green. The other sweep/has_address sites are
+	// fine because they run after the scan, not before.
 	std::array blocks = {
 		std::reference_wrapper{ heap.slab().block_16() },
 		std::reference_wrapper{ heap.slab().block_32() },
@@ -262,21 +287,8 @@ void MarkSweepGC::sweep(Heap &heap) const
 {
 	spdlog::trace("MarkSweepGC::sweep start");
 
-	// TODO: once the ideal block sizes are fixed there should be an iterator
-	//       returning a list of all blocks
-	std::array blocks = {
-		std::reference_wrapper{ heap.slab().block_16() },
-		std::reference_wrapper{ heap.slab().block_32() },
-		std::reference_wrapper{ heap.slab().block_64() },
-		std::reference_wrapper{ heap.slab().block_128() },
-		std::reference_wrapper{ heap.slab().block_256() },
-		std::reference_wrapper{ heap.slab().block_512() },
-		std::reference_wrapper{ heap.slab().block_1024() },
-		std::reference_wrapper{ heap.slab().block_2048() },
-	};
-	// sweep all the dead objects
-	for (const auto &block : blocks) {
-		for (auto &chunk : block.get()->chunks()) {
+	heap.slab().for_each_block([this, &heap](Block &block) {
+		for (auto &chunk : block.chunks()) {
 			chunk.for_each_cell_alive([this, &chunk, &heap](uint8_t *memory) {
 				auto *header = bit_cast<GarbageCollected *>(memory);
 				if (header->white()) {
@@ -293,27 +305,29 @@ void MarkSweepGC::sweep(Heap &heap) const
 				}
 			});
 		}
-	}
+	});
 	spdlog::trace("MarkSweepGC::sweep done");
 }
 
 
-void MarkSweepGC::run(Heap &heap) const
+void MarkSweepGC::mark_and_sweep(Heap &heap)
 {
-	if (m_pause) { return; }
-	if (++m_iterations_since_last_sweep < m_frequency) { return; }
-
 	mark_all_cell_unreachable(heap);
-
 	auto roots = collect_roots(heap);
-
 	mark_all_live_objects(heap, std::move(roots));
-
 	sweep(heap);
-
 	m_iterations_since_last_sweep = 0;
 }
 
+void MarkSweepGC::run(Heap &heap)
+{
+	if (m_pause) { return; }
+	if (++m_iterations_since_last_sweep < m_frequency) { return; }
+	mark_and_sweep(heap);
+}
+
+void MarkSweepGC::force_run(Heap &heap) { mark_and_sweep(heap); }
+
 void MarkSweepGC::resume()
 {
 	ASSERT(!is_active());

src/memory/GarbageCollector.hpp

@@ -64,7 +64,15 @@ class GarbageCollector
 {
   public:
 	virtual ~GarbageCollector() = default;
-	virtual void run(Heap &) const = 0;
+	// Not const: a GC pass advances the collector's per-call cadence
+	// counter, flips every cell's mark colour, and may destroy a large
+	// chunk of the heap. Pretending the GC instance is immutable across
+	// the call is a const-correctness lie that this signature avoids.
+	virtual void run(Heap &) = 0;
+	// Unconditional collection. Bypasses both the pause flag and the
+	// cadence counter; used by gc.collect() so user code can force a
+	// full sweep even while the GC has been disabled.
+	virtual void force_run(Heap &) = 0;
 	virtual void resume() = 0;
 	virtual void pause() = 0;
 	virtual bool is_active() const = 0;
@@ -81,7 +89,8 @@ class MarkSweepGC : public GarbageCollector
 {
   public:
 	MarkSweepGC();
-	void run(Heap &) const override;
+	void run(Heap &) override;
+	void force_run(Heap &) override;
 	void resume() override;
 	void pause() override;
 	bool is_active() const override;
@@ -92,7 +101,11 @@ class MarkSweepGC : public GarbageCollector
 	void sweep(Heap &heap) const;
 
   private:
+	void mark_and_sweep(Heap &);
+
+	// Lazily initialised once on first collect_roots; afterwards immutable.
+	// `mutable` is the textbook one-time-cache use case.
 	mutable uint8_t *m_stack_bottom{ nullptr };
-	mutable size_t m_iterations_since_last_sweep{ 0 };
+	size_t m_iterations_since_last_sweep{ 0 };
 	bool m_pause{ false };
 };

src/memory/GarbageCollector_tests.cpp

@@ -77,4 +77,52 @@ TEST_F(TestHeap, GarbageCollectorDeallocatesGCPointersWhenStackFrameIsPopped)
 	m_heap->collect_garbage();
 
 	ASSERT_EQ(g_counter, 5);
-}
+}
+
+namespace {
+
+struct Cycle : Cell
+{
+	Cycle *other{ nullptr };
+	int64_t &counter;
+	explicit Cycle(int64_t &counter_) : counter(counter_) {}
+	~Cycle() { counter++; }
+	std::string to_string() const override { return "Cycle"; }
+	void visit_graph(Visitor &visitor) override
+	{
+		visitor.visit(*this);
+		if (other) { visitor.visit(*other); }
+	}
+};
+
+// Allocates two mutually-referencing Cycle objects in a popped frame so
+// the conservative stack scan can't keep them alive past return.
+#if defined(__clang__)
+__attribute__((noinline, optnone)) void allocate_cycle_in_popped_frame(Heap &heap, int64_t &counter)
+#elif defined(__GNUC__)
+__attribute__((noinline, optimize("-O0"))) void allocate_cycle_in_popped_frame(Heap &heap,
+	int64_t &counter)
+#endif
+{
+	auto *a = heap.allocate<Cycle>(counter);
+	auto *b = heap.allocate<Cycle>(counter);
+	a->other = b;
+	b->other = a;
+}
+
+}// namespace
+
+TEST_F(TestHeap, MutuallyReferencingObjectsAreCollected)
+{
+	// Two Cycle objects pointing at each other should be collected once
+	// no external roots reach them — this is the canonical mark-sweep
+	// reachability test that a refcount-only GC would fail.
+	int64_t counter = 0;
+	m_heap->garbage_collector().set_frequency(1);
+
+	allocate_cycle_in_popped_frame(*m_heap, counter);
+
+	m_heap->collect_garbage();
+
+	ASSERT_EQ(counter, 2);
+}

src/memory/Heap.cpp

@@ -137,24 +137,17 @@ void Block::deallocate(uint8_t *ptr)
 
 bool Slab::has_address(uint8_t *address) const
 {
-	// TODO: once the ideal block sizes are fixed there should be an iterator
-	//       returning a list of all blocks
-	std::array blocks = {
-		block16.get(),
-		block32.get(),
-		block64.get(),
-		block128.get(),
-		block256.get(),
-		block512.get(),
-		block1024.get(),
-		block2048.get(),
-	};
-	for (const auto &block : blocks) {
-		for (auto &chunk : block->chunks()) {
-			if (chunk.has_address(address)) { return true; }
+	bool found = false;
+	for_each_block([&](const Block &block) {
+		if (found) { return; }
+		for (const auto &chunk : block.chunks()) {
+			if (chunk.has_address(address)) {
+				found = true;
+				return;
+			}
 		}
-	}
-	return false;
+	});
+	return found;
 }
 
 Heap::Heap()