Skip to content

Commit a77c978

Browse files
fangererfangerer
committed
Fix: remove weakly referenced objects from GC lists.
1 parent f072015 commit a77c978

File tree

4 files changed

+64
-11
lines changed

4 files changed

+64
-11
lines changed

‎graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_gc.py‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,7 @@ def test_native_class(self):
192192
ID_OBJ12 = 10
193193
ID_OBJ13 = 11
194194
ID_OBJ14 = 12
195+
ID_OBJ15 = 13
195196

196197
# don't rely on deterministic Java GC behavior by default on GraalPy
197198
RELY_ON_GC = os.environ.get("RELY_ON_GC", not GRAALPY)
@@ -204,13 +205,15 @@ def test_native_class(self):
204205

205206
if GRAALPY:
206207
get_handle_table_id = __graalpython__.get_handle_table_id
208+
storage_to_native = __graalpython__.storage_to_native
207209
def assert_is_strong(x): assert __graalpython__.is_strong_handle_table_ref(x)
208210
def assert_is_weak(x): assert not __graalpython__.is_strong_handle_table_ref(x)
209211
else:
210212
# just that the test is compatible with CPython
211-
def get_handle_table_id(object): return -1
212-
def assert_is_strong(x): pass
213-
def assert_is_weak(x): pass
213+
def get_handle_table_id(_): return -1
214+
def storage_to_native(_): pass
215+
def assert_is_strong(_): pass
216+
def assert_is_weak(_): pass
214217

215218
class TestGCRefCycles(unittest.TestCase):
216219
def _trigger_gc(self):
@@ -357,6 +360,7 @@ def assert_is_freed(id): pass
357360
obj10 = TestCycle0(ID_OBJ10)
358361
obj11 = TestCycle0(ID_OBJ11)
359362
obj14 = TestCycle0(ID_OBJ14)
363+
obj15 = TestCycle0(ID_OBJ15)
360364

361365
# Legend
362366
# '=>'
@@ -462,6 +466,19 @@ def assert_is_freed(id): pass
462466
htid_l4 = get_handle_table_id(l4)
463467
del l4
464468

469+
# establish cycle: l5 => obj15 =ht=> l5
470+
# update_refs: 11 1
471+
# subtract_refs: 10 0
472+
# move_unreachable: 10 11
473+
# update_refs: 11 11
474+
# subtract_refs: 10 10
475+
# commit_weak_cand: l5 => obj15 =ht-> l5
476+
l5 = [obj15]
477+
storage_to_native(l5)
478+
obj15.set_obj(l5)
479+
htid_l5 = get_handle_table_id(l5)
480+
del obj15
481+
465482
# everything should still be alive
466483
assert_is_alive(ID_OBJ2)
467484
assert_is_alive(ID_OBJ3)
@@ -474,16 +491,19 @@ def assert_is_freed(id): pass
474491
assert_is_alive(ID_OBJ10)
475492
assert_is_alive(ID_OBJ11)
476493
assert_is_alive(ID_OBJ14)
494+
assert_is_alive(ID_OBJ15)
477495
assert_is_strong(htid_l)
478496
assert_is_strong(htid_l1)
479497
assert_is_strong(htid_l2)
480498
assert_is_strong(htid_l3)
481499
assert_is_strong(htid_l4)
500+
assert_is_strong(htid_l5)
482501
assert_is_strong(htid_d0)
483502

484503
del obj2, l, obj3
485504
del obj4, obj5
486505
del obj6, d0, obj7
506+
del l5
487507

488508
####################################### GC #######################################
489509
self._trigger_gc()
@@ -511,6 +531,7 @@ def assert_is_freed(id): pass
511531
assert_is_strong(htid_l1)
512532
assert_is_strong(htid_l4)
513533
assert_is_strong(htid_d0)
534+
assert_is_freed(ID_OBJ15)
514535

515536
rescued_obj4 = l1[0]
516537
del l1

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1555,7 +1555,7 @@ static Object doNative(Object weakCandidates,
15551555
GC_LOGGER.fine(PythonUtils.formatJString("Transitioning to weak reference to break a reference cycle for %s, refcount=%d",
15561556
abstractObjectNativeWrapper.ref, abstractObjectNativeWrapper.getRefCount()));
15571557
}
1558-
updateRefNode.clearStrongRef(inliningTarget, abstractObjectNativeWrapper);
1558+
updateRefNode.clearStrongRefButKeepInGCList(inliningTarget, abstractObjectNativeWrapper);
15591559
}
15601560

15611561
// next = GC_NEXT(gc)

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiGCSupport.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444

4545
import java.util.logging.Level;
4646

47+
import com.oracle.graal.python.PythonLanguage;
4748
import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.GCListRemoveNodeGen;
4849
import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.PyObjectGCDelNodeGen;
4950
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.PExternalFunctionWrapper;
@@ -55,6 +56,7 @@
5556
import com.oracle.graal.python.builtins.objects.cext.structs.CStructAccess.FreeNode;
5657
import com.oracle.graal.python.builtins.objects.cext.structs.CStructs;
5758
import com.oracle.graal.python.runtime.PythonContext;
59+
import com.oracle.graal.python.runtime.PythonOptions;
5860
import com.oracle.graal.python.util.PythonUtils;
5961
import com.oracle.truffle.api.dsl.Cached;
6062
import com.oracle.truffle.api.dsl.GenerateCached;
@@ -165,12 +167,24 @@ public static long executeUncached(long opUntagged) {
165167
return GCListRemoveNodeGen.getUncached().execute(null, opUntagged);
166168
}
167169

170+
/**
171+
* Remove the Python object denoted by the given {@code PyObject*} pointer from the GC list
172+
* it is currently contained in. This will apply {@code AS_GC(op)} first.
173+
*/
174+
public final void executeOp(Node inliningTarget, long op) {
175+
if (PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
176+
execute(inliningTarget, HandlePointerConverter.pointerToStub(op));
177+
}
178+
}
179+
168180
public abstract long execute(Node inliningTarget, long opUntagged);
169181

170182
@Specialization
171183
static long doGeneric(long opUntagged,
172184
@Cached(inline = false) CStructAccess.ReadI64Node readI64Node,
173185
@Cached(inline = false) CStructAccess.WriteLongNode writeLongNode) {
186+
assert PythonLanguage.get(null).getEngineOption(PythonOptions.PythonGC);
187+
174188
// issue a log message before doing the first memory access
175189
if (GC_LOGGER.isLoggable(Level.FINER)) {
176190
GC_LOGGER.finer(PythonUtils.formatJString("attempting to remove 0x%x from GC generation", opUntagged));

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2152,30 +2152,48 @@ static Object doIt(Object object) {
21522152
public abstract static class UpdateStrongRefNode extends Node {
21532153

21542154
public final void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, long refCount) {
2155-
execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT);
2155+
execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT, false);
21562156
}
21572157

2158-
public final void clearStrongRef(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
2159-
execute(inliningTarget, wrapper, false);
2158+
/**
2159+
* Makes the handle table reference of the given wrapper weak but keeps the native object
2160+
* stub in the GC list (if currently contained in any). The only valid use case for this
2161+
* method is when iterating over all objects of a GC list, calling this method on each
2162+
* object and in the end, dropping the whole GC list.
2163+
*/
2164+
public final void clearStrongRefButKeepInGCList(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
2165+
execute(inliningTarget, wrapper, false, true);
21602166
}
21612167

2162-
public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong);
2168+
public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList);
21632169

21642170
@Specialization
2165-
static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong,
2171+
static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList,
21662172
@Cached InlinedConditionProfile hasRefProfile,
2167-
@Cached PyObjectGCTrackNode gcTrackNode) {
2173+
@Cached PyObjectGCTrackNode gcTrackNode,
2174+
@Cached GCListRemoveNode gcListRemoveNode) {
2175+
assert CompilerDirectives.isPartialEvaluationConstant(keepInGcList);
2176+
21682177
PythonObjectReference ref;
21692178
if (hasRefProfile.profile(inliningTarget, (ref = wrapper.ref) != null)) {
21702179
assert ref.pointer == wrapper.getNativePointer();
21712180
if (setStrong && !ref.isStrongReference()) {
21722181
ref.setStrongReference(wrapper);
21732182
if (ref.gc && PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
21742183
// gc = AS_GC(op)
2175-
long gc = wrapper.getNativePointer() - CStructs.PyGC_Head.size();
2184+
long gc = ref.pointer - CStructs.PyGC_Head.size();
21762185
gcTrackNode.execute(inliningTarget, gc);
21772186
}
21782187
} else if (!setStrong && ref.isStrongReference()) {
2188+
/*
2189+
* As soon as the reference is made weak, we remove it from the GC list because
2190+
* there are ways to iterate a GC list (e.g. 'PyUnstable_GC_VisitObjects') and
2191+
* while doing so, the objects may be accessed. Since weakly referenced objects
2192+
* may die any time, this could lead to dangling pointers being used.
2193+
*/
2194+
if (!keepInGcList && ref.gc) {
2195+
gcListRemoveNode.executeOp(inliningTarget, ref.pointer);
2196+
}
21792197
ref.setStrongReference(null);
21802198
}
21812199
}

0 commit comments

Comments
 (0)