Allocate Global with uncollectable memory so GC keeps live references by makachanm · Pull Request #509 · Samsung/walrus

makachanm · 2026-10-06T02:32:06Z

ksh8281

Thanks for the PR!

I have a concern about potential memory leaks due to conservative GC scanning. Since Wasm globals write arbitrary raw numbers (i32, i64, f32, etc.), allocating them as uncollectable memory will lead to severe false pointer retention (leaks) as the GC scans the entire block conservatively.

Could you please update this to use a custom mark procedure (proc) to create a GC kind and only mark/scan valid pointer slots?

Also, please update gcutil to the latest branch (bdwgc_8_3_pre_260715) for the time being.

zherczeg · 2026-10-06T04:34:00Z

I would keep the GC object as malloced. Its value should be an union, which contains primitive values, or a pointer to a pointer for references. The referenced pointer is allocated by GC.

clover2123 · 2026-10-06T05:32:49Z

@ksh8281 Do you mean GC_descr used in escargot like below?
https://github.com/Samsung/escargot/blob/a3c837522f553c1df44bff582816f878da3704b2/src/runtime/MapObject.cpp#L38-L50

clover2123 · 2026-10-06T05:41:32Z

@zherczeg
Would you elaborate more about your idea?
If a GC pointer is newly allocated in a Global object, can we recognize it and just call addRef?

zherczeg · 2026-10-06T05:57:34Z

A global type is known at allocation time, so the global type can tell that we have a primitive value or a pointer. A pointer is a pointer to a "vector", which has a single pointer value. So it is kind of a table with a single value, but this table is allocated by GC.

This way globals keeps their current form except for references when GC is enabled.

zherczeg · 2026-10-06T06:00:01Z

Another option is having a real "global vector" for references, which is stored in the Store. This vector has used and unused slots. Releasing globals is more complex that way.

makachanm · 2026-10-06T06:10:49Z

Is enough to use GC_MALLOC_ATOMIC_UNCOLLECTABLE?
or it needs rework about entire Global object?

zherczeg · 2026-10-06T08:17:30Z

I am not sure I understand the question. The global object should remain mostly the same. It is a store object, and it cannot be on the webassembly heap, so it does not need to be handled by GC.

ksh8281 · 2026-10-06T08:40:31Z

@makachanm @clover2123

Regarding the Global allocation and how we should handle the pointer tracking with bdwgc, here is my feedback on the proposed changes:

If we can determine the allocation properties at allocation time:
Case A: bdwgc needs to track the allocation itself, but the internal content does NOT contain any pointers.

In this case, using GC_MALLOC_ATOMIC_UNCOLLECTABLE is the most appropriate choice to avoid unnecessary scanning.

Case B: The internal content indeed contains pointers that the GC must traverse.

We could go with typed GC (like the one linked by clover1213).

However, since the kind value is very small here, there is practically no risk of conservative GC misinterpreting it as an arbitrary pointer. Therefore, if the Value is just a simple raw pointer, using GC_MALLOC_UNCOLLECTABLE should be more than enough.

If we CANNOT 100% determine this at allocation time:
We should introduce a new custom kind and register a marking procedure (proc) to handle this dynamically.

Similar to how it is implemented in Escargot's CustomAllocator.cpp, we should write a custom proc that evaluates the object at runtime, determines if it behaves as a pointer, and explicitly notifies bdwgc (i.e., "this is a pointer") only when it is actually identified as one.

Let me know what you think about this approach!

zherczeg · 2026-10-06T08:51:58Z

Globals are not real objects, they are objects in Walrus only for practical reasons. They represent values that can be imported/exported by WebAssembly instances. They are stored in a Store, and they should have a reference counter, not just stored until the Store is destroyed. When a global object is created, everything is known about it, including its type. The type is important for importing/exporting checks.

A global object could be allocated by GC_MALLOC_UNCOLLECTABLE / GC_FREE, if it contains a reference, or regular malloc/free if it does not. Or it can have a pointer to a single reference. The latter increases the runtime a bit.

makachanm · 2026-10-06T12:12:57Z

For the code consistency, GCArray already uses Value::isRefType(valueType) ? GC_MALLOC(totalSize) : GC_MALLOC_ATOMIC(totalSize) to allocate memory with type check that implies this value can't be a reference when normally allocated.

I think if the type is constant, this kind of method will prevent GC confusion.

ksh8281 · 2026-10-06T12:42:14Z

Yes, that makes sense. Since a Global’s type is known at creation and does not change, we can choose the allocation strategy based on its type. My earlier suggestion about a custom marking procedure is unnecessary in this case.

The same type-based approach as GCArray applies, but the allocation must also respect the Global’s explicitly managed lifetime. Reference globals can use GC_MALLOC_UNCOLLECTABLE with GC_FREE, while non-reference globals can keep malloc/free. If we want both paths to use GC allocation, GC_MALLOC_ATOMIC_UNCOLLECTABLE is suitable for the non-reference case.

zherczeg · 2026-10-07T03:09:17Z

+Global* Global::createGlobal(Store* store, const Value& value, const MutableType& type)
+{
+#ifdef ENABLE_GC
+    void* mem = Value::isRefType(type.type()) ? GC_MALLOC_UNCOLLECTABLE(sizeof(Global)) : GC_MALLOC_ATOMIC_UNCOLLECTABLE(sizeof(Global));


Is GC_MALLOC_ATOMIC_UNCOLLECTABLE is a regular malloc? Is it recommended instead of a malloc?

GC_MALLOC_ATOMIC_UNCOLLECTABLE is uncollectable GC allocation.

Issue #502 is caused by early collection of global object that originally written in new allocation.

Marking uncollctable keeps object safe.

That is clear to me. My question is GC_MALLOC_ATOMIC_UNCOLLECTABLE is better than regular malloc? Why?

Using regular free(or delete) is possible, but it needs separate memory deallocation path into two ways.

I just think that is not a smooth way for consistency.

Ok. I hope these blocks are not difficult to process for GC.

zherczeg · 2026-10-07T04:05:41Z

+
+#ifdef ENABLE_GC
+    void operator delete(void* ptr);
+#endif


Longer term task: Store objects should be freed only by Store, so we might need to make their free operations private. And we should also add a reference counting.

zherczeg · 2026-10-07T05:58:40Z

+      (drop (array.new_default $a (i32.const 65536)))
+      (local.set $i (i32.add (local.get $i) (i32.const 1)))
+      (br_if $l (i32.lt_u (local.get $i) (i32.const 3000))))
+    (struct.get $s 0 (global.get $g))))


Do you need this large constant? Is this a long running test?

Maybe we should add a walrus_gc builtin rather than these large consts.

The manual call of builtin seems makes false positive. (I'm currently testing it. Just added builtin function.) We need mess up the memory to reproduce this bug.

That is very strange. You need a short loop to remove the reference from the stack, but the referenced object should go away.

@zherczeg
IMO this code is enough to test the correct GC for the global object.
In the loop, arrays are repeatedly allocated and dropped to trigger the GC.
Even more, an explicit GC call is invoked.
So, this code could confirm that GC tracking is correctly executed for the global object.

Thank you for explaining. My only worry is that 305419896 might make the test long running, and there is no guarantee that GC is triggered depending on the available memory. So I prefer a forced GC. A small loop should be enough to clear the stack from the earlier references, and GC should free those objects. However, @makachanm said it does not work, and I asked why.

clover2123 · 2026-10-07T07:37:16Z


+#ifdef ENABLE_GC
+#include "GCUtil.h"
+#include <new>


Is this include <new> necessary?

makachanm · 2026-10-08T08:02:53Z

Do I have more to do for this patch? (Is there are something I missing about?)

clover2123 · 2026-10-08T08:12:39Z

+                    ft,
+                    [](ExecutionState& state, Value* argv, Value* result, void* data) {
+#ifdef ENABLE_GC
+                        GC_gcollect();


Would you change this function to GC_gcollect_and_unmap();?
This gc function is more powerful because it additionally returns the reclaimed memory to OS

zherczeg · 2026-10-08T08:46:09Z

                    [](ExecutionState& state, Value* argv, Value* result, void* data) {
                    },
                    nullptr));
+            } else if (import->fieldName() == "gc") {


I would prefer walrus_gc, because the others are spec test definitions, and if they introduce a gc in the future, it would be good to detect it. It is unlikely that they introduce walrus_gc.

zherczeg

LGTM. Only the loop of 305419896 is strange for me.

makachanm requested review from clover2123 and ksh8281 as code owners October 6, 2026 02:32

ksh8281 requested changes Oct 6, 2026

View reviewed changes

Update GCutil to bdwgc_8_3_pre_260715

99171c3

makachanm force-pushed the bugfix branch from b362c7a to f798369 Compare October 7, 2026 00:55

makachanm mentioned this pull request Oct 7, 2026

[Bug] GC: references held in globals are not roots and get freed while live #502

Open

zherczeg reviewed Oct 7, 2026

View reviewed changes

makachanm force-pushed the bugfix branch from f798369 to 3ea020e Compare October 7, 2026 04:30

zherczeg reviewed Oct 7, 2026

View reviewed changes

makachanm force-pushed the bugfix branch from 3ea020e to 07b2ee3 Compare October 7, 2026 07:13

clover2123 reviewed Oct 7, 2026

View reviewed changes

makachanm force-pushed the bugfix branch 2 times, most recently from 009cbd2 to b8b54c7 Compare October 7, 2026 08:19

clover2123 reviewed Oct 8, 2026

View reviewed changes

makachanm force-pushed the bugfix branch from b8b54c7 to 8677972 Compare October 8, 2026 08:38

zherczeg reviewed Oct 8, 2026

View reviewed changes

Fix GC collecting live references stored in globals

e788f0a

makachanm force-pushed the bugfix branch from 8677972 to e788f0a Compare October 8, 2026 08:56

zherczeg approved these changes Oct 8, 2026

View reviewed changes

Conversation

makachanm commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksh8281 left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Oct 6, 2026

Uh oh!

clover2123 commented Oct 6, 2026

Uh oh!

clover2123 commented Oct 6, 2026

Uh oh!

zherczeg commented Oct 6, 2026

Uh oh!

zherczeg commented Oct 6, 2026

Uh oh!

makachanm commented Oct 6, 2026

Uh oh!

zherczeg commented Oct 6, 2026

Uh oh!

ksh8281 commented Oct 6, 2026

Uh oh!

zherczeg commented Oct 6, 2026

Uh oh!

makachanm commented Oct 6, 2026

Uh oh!

ksh8281 commented Oct 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makachanm Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makachanm commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

makachanm commented Oct 6, 2026 •

edited

Loading

makachanm Oct 7, 2026 •

edited

Loading

makachanm commented Oct 8, 2026 •

edited

Loading