Skip to content

Commit 85ab9cb

Browse files
msimacekmsimacek
committed
Acquire GIL around C upcalls
1 parent 625a9de commit 85ab9cb

File tree

6 files changed

+32
-17
lines changed

6 files changed

+32
-17
lines changed

‎graalpython/com.oracle.graal.python.cext/modules/_sqlite/connection.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,8 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database,
199199
// Create and configure SQLite database object.
200200
sqlite3 *db;
201201
int rc;
202-
// GraalPy change: workaround for GR-51314, revert to CPython original once fixed
203-
char* cstr = PyBytes_AS_STRING(bytes);
204202
Py_BEGIN_ALLOW_THREADS
205-
rc = sqlite3_open_v2(cstr, &db,
203+
rc = sqlite3_open_v2(PyBytes_AS_STRING(bytes), &db,
206204
SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE |
207205
(uri ? SQLITE_OPEN_URI : 0), NULL);
208206
if (rc == SQLITE_OK) {

‎graalpython/com.oracle.graal.python.processor/src/com/oracle/graal/python/processor/CApiBuiltinsProcessor.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,17 @@ private static final class CApiBuiltinDesc {
168168
public final String name;
169169
public final VariableElement[] arguments;
170170
public final VariableElement returnType;
171+
public final boolean acquireGil;
171172
public final String call;
172173
public final String factory;
173174
public int id;
174175

175-
public CApiBuiltinDesc(Element origin, String name, VariableElement returnType, VariableElement[] arguments, String call, String factory) {
176+
public CApiBuiltinDesc(Element origin, String name, VariableElement returnType, VariableElement[] arguments, boolean acquireGil, String call, String factory) {
176177
this.origin = origin;
177178
this.name = name;
178179
this.returnType = returnType;
179180
this.arguments = arguments;
181+
this.acquireGil = acquireGil;
180182
this.call = call;
181183
this.factory = factory;
182184
}
@@ -356,11 +358,12 @@ private void addCApiBuiltins(RoundEnvironment re, List<CApiBuiltinDesc> javaBuil
356358
name = builtinName;
357359
}
358360
var ret = findValue(builtin, "ret", VariableElement.class);
361+
boolean acquireGil = findValue(builtin, "acquireGil", Boolean.class);
359362
String call = name(findValue(builtin, "call", VariableElement.class));
360363
// boolean inlined = findValue(builtin, "inlined", Boolean.class);
361364
VariableElement[] args = findValues(builtin, "args", VariableElement.class).toArray(new VariableElement[0]);
362365
if (((TypeElement) element).getQualifiedName().toString().equals("com.oracle.graal.python.builtins.objects.cext.capi.CApiFunction.Dummy")) {
363-
additionalBuiltins.add(new CApiBuiltinDesc(element, builtinName, ret, args, call, null));
366+
additionalBuiltins.add(new CApiBuiltinDesc(element, builtinName, ret, args, acquireGil, call, null));
364367
} else {
365368
if (!isValidReturnType(ret)) {
366369
processingEnv.getMessager().printError(
@@ -387,7 +390,7 @@ private void addCApiBuiltins(RoundEnvironment re, List<CApiBuiltinDesc> javaBuil
387390
genName += "NodeGen";
388391
}
389392
verifyNodeClass(((TypeElement) element), builtin);
390-
javaBuiltins.add(new CApiBuiltinDesc(element, name, ret, args, call, genName));
393+
javaBuiltins.add(new CApiBuiltinDesc(element, name, ret, args, acquireGil, call, genName));
391394
}
392395
}
393396
}
@@ -634,7 +637,7 @@ private void generateBuiltinRegistry(List<CApiBuiltinDesc> javaBuiltins) throws
634637
for (var builtin : javaBuiltins) {
635638
String argString = Arrays.stream(builtin.arguments).map(b -> "ArgDescriptor." + b).collect(Collectors.joining(", "));
636639
lines.add(" public static final CApiBuiltinExecutable " + builtin.name + " = new CApiBuiltinExecutable(\"" + builtin.name + "\", CApiCallPath." + builtin.call + ", ArgDescriptor." +
637-
builtin.returnType + ", new ArgDescriptor[]{" + argString + "}, " + builtin.id + ");");
640+
builtin.returnType + ", new ArgDescriptor[]{" + argString + "}, " + builtin.acquireGil + ", " + builtin.id + ");");
638641
}
639642
lines.add("");
640643
lines.add(" public static final CApiBuiltinExecutable[] builtins = {");

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@
172172
import com.oracle.graal.python.nodes.object.GetClassNode;
173173
import com.oracle.graal.python.nodes.statement.AbstractImportNode;
174174
import com.oracle.graal.python.nodes.util.CastToJavaIntExactNode;
175+
import com.oracle.graal.python.runtime.GilNode;
175176
import com.oracle.graal.python.runtime.IndirectCallData;
176177
import com.oracle.graal.python.runtime.PosixSupportLibrary;
177178
import com.oracle.graal.python.runtime.PosixSupportLibrary.PosixException;
@@ -584,17 +585,19 @@ public static final class CApiBuiltinExecutable implements TruffleObject {
584585
private final CApiTiming timing;
585586
private final ArgDescriptor ret;
586587
private final ArgDescriptor[] args;
588+
private final boolean acquireGil;
587589
@CompilationFinal private CallTarget callTarget;
588590
private final CApiCallPath call;
589591
private final String name;
590592
private final int id;
591593

592-
public CApiBuiltinExecutable(String name, CApiCallPath call, ArgDescriptor ret, ArgDescriptor[] args, int id) {
594+
public CApiBuiltinExecutable(String name, CApiCallPath call, ArgDescriptor ret, ArgDescriptor[] args, boolean acquireGil, int id) {
593595
this.timing = CApiTiming.create(false, name);
594596
this.name = name;
595597
this.call = call;
596598
this.ret = ret;
597599
this.args = args;
600+
this.acquireGil = acquireGil;
598601
this.id = id;
599602
}
600603

@@ -619,6 +622,10 @@ public String name() {
619622
return name;
620623
}
621624

625+
public boolean acquireGil() {
626+
return acquireGil;
627+
}
628+
622629
CExtToNativeNode createRetNode() {
623630
return ret.createPythonToNativeNode();
624631
}
@@ -773,6 +780,7 @@ public static ExecuteCApiBuiltinNode getUncached(@SuppressWarnings("unused") CAp
773780

774781
static final class CachedExecuteCApiBuiltinNode extends ExecuteCApiBuiltinNode {
775782
private final CApiBuiltinExecutable cachedSelf;
783+
@Child private GilNode gilNode = GilNode.create();
776784
@Child private CExtToNativeNode retNode;
777785
@Children private final CExtToJavaNode[] argNodes;
778786
@Child private CApiBuiltinNode builtinNode;
@@ -788,6 +796,7 @@ static final class CachedExecuteCApiBuiltinNode extends ExecuteCApiBuiltinNode {
788796

789797
@Override
790798
Object execute(CApiBuiltinExecutable self, Object[] arguments) {
799+
boolean wasAcquired = self.acquireGil() && gilNode.acquire();
791800
CApiTiming.enter();
792801
try {
793802
try {
@@ -827,6 +836,7 @@ Object execute(CApiBuiltinExecutable self, Object[] arguments) {
827836
throw CompilerDirectives.shouldNotReachHere("return type while handling PException: " + cachedSelf.getRetDescriptor() + " in " + self.name);
828837
}
829838
} finally {
839+
gilNode.release(wasAcquired);
830840
CApiTiming.exit(self.timing);
831841
}
832842
}
@@ -935,7 +945,11 @@ public enum CApiCallPath {
935945
*/
936946
boolean inlined() default false;
937947

938-
boolean acquiresGIL() default true;
948+
/**
949+
* Whether we need to acquire GIL around the call. Since our conversion nodes currently
950+
* assume GIL, this should be only set to false by GIL-manipulating builtins.
951+
*/
952+
boolean acquireGil() default true;
939953

940954
/**
941955
* @see CApiCallPath

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888

8989
public final class PythonCextCEvalBuiltins {
9090

91-
@CApiBuiltin(ret = PyThreadState, args = {}, acquiresGIL = false, call = Direct)
91+
@CApiBuiltin(ret = PyThreadState, args = {}, acquireGil = false, call = Direct)
9292
abstract static class PyEval_SaveThread extends CApiNullaryBuiltinNode {
9393
private static final TruffleLogger LOGGER = CApiContext.getLogger(PyEval_SaveThread.class);
9494

@@ -102,7 +102,7 @@ static Object save(@Cached GilNode gil) {
102102
}
103103
}
104104

105-
@CApiBuiltin(ret = Void, args = {PyThreadState}, acquiresGIL = false, call = Direct)
105+
@CApiBuiltin(ret = Void, args = {PyThreadState}, acquireGil = false, call = Direct)
106106
abstract static class PyEval_RestoreThread extends CApiUnaryBuiltinNode {
107107
private static final TruffleLogger LOGGER = CApiContext.getLogger(PyEval_RestoreThread.class);
108108

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@
7676

7777
public final class PythonCextPyStateBuiltins {
7878

79-
@CApiBuiltin(ret = Int, args = {}, acquiresGIL = false, call = Direct)
79+
@CApiBuiltin(ret = Int, args = {}, acquireGil = false, call = Direct)
8080
abstract static class PyTruffleGILState_Check extends CApiNullaryBuiltinNode {
8181

8282
@Specialization
@@ -85,7 +85,7 @@ Object check() {
8585
}
8686
}
8787

88-
@CApiBuiltin(ret = Int, args = {}, acquiresGIL = false, call = Direct)
88+
@CApiBuiltin(ret = Int, args = {}, acquireGil = false, call = Direct)
8989
abstract static class PyTruffleGILState_Ensure extends CApiNullaryBuiltinNode {
9090

9191
@Specialization
@@ -95,7 +95,7 @@ static Object save(@Cached GilNode gil) {
9595
}
9696
}
9797

98-
@CApiBuiltin(ret = Void, args = {}, acquiresGIL = false, call = Direct)
98+
@CApiBuiltin(ret = Void, args = {}, acquireGil = false, call = Direct)
9999
abstract static class PyTruffleGILState_Release extends CApiNullaryBuiltinNode {
100100

101101
@Specialization

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ public final class CApiFunction {
186186
/*
187187
* Functions that are implemented as C code that can be executed both in native and in Sulong:
188188
*/
189-
@CApiBuiltin(name = "PyGILState_Check", ret = Int, args = {}, acquiresGIL = false, call = CImpl)
189+
@CApiBuiltin(name = "PyGILState_Check", ret = Int, args = {}, acquireGil = false, call = CImpl)
190190
@CApiBuiltin(name = "PyArg_Parse", ret = Int, args = {PyObject, ConstCharPtrAsTruffleString, VARARGS}, call = CImpl)
191191
@CApiBuiltin(name = "PyArg_ParseTuple", ret = Int, args = {PyObject, ConstCharPtrAsTruffleString, VARARGS}, call = CImpl)
192192
@CApiBuiltin(name = "PyArg_ParseTupleAndKeywords", ret = Int, args = {PyObject, PyObject, ConstCharPtrAsTruffleString, CHAR_PTR_LIST, VARARGS}, call = CImpl)
@@ -284,9 +284,9 @@ public final class CApiFunction {
284284
@CApiBuiltin(name = "PyFloat_Unpack2", ret = Double, args = {ConstCharPtrAsTruffleString, Int}, call = CImpl)
285285
@CApiBuiltin(name = "PyFloat_Unpack4", ret = Double, args = {ConstCharPtrAsTruffleString, Int}, call = CImpl)
286286
@CApiBuiltin(name = "PyFloat_Unpack8", ret = Double, args = {ConstCharPtrAsTruffleString, Int}, call = CImpl)
287-
@CApiBuiltin(name = "PyGILState_Ensure", ret = PY_GIL_STATE_STATE, args = {}, acquiresGIL = false, call = CImpl)
287+
@CApiBuiltin(name = "PyGILState_Ensure", ret = PY_GIL_STATE_STATE, args = {}, acquireGil = false, call = CImpl)
288288
@CApiBuiltin(name = "PyGILState_GetThisThreadState", ret = PyThreadState, args = {}, call = CImpl)
289-
@CApiBuiltin(name = "PyGILState_Release", ret = Void, args = {PY_GIL_STATE_STATE}, acquiresGIL = false, call = CImpl)
289+
@CApiBuiltin(name = "PyGILState_Release", ret = Void, args = {PY_GIL_STATE_STATE}, acquireGil = false, call = CImpl)
290290
@CApiBuiltin(name = "PyGen_New", ret = PyObject, args = {PyFrameObject}, call = CImpl)
291291
@CApiBuiltin(name = "PyGen_NewWithQualName", ret = PyObject, args = {PyFrameObject, PyObject, PyObject}, call = CImpl)
292292
@CApiBuiltin(name = "PyImport_AddModule", ret = PyObject, args = {ConstCharPtrAsTruffleString}, call = CImpl)

0 commit comments

Comments
 (0)