[lldb] Reimplment PyRun_String using the Python stable C API #151761
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesReimplement Part of #151617 Full diff: https://github.com/llvm/llvm-project/pull/151761.diff 3 Files Affected:
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 98c9b61c11980..a2832bd16aaf3 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -498,9 +498,7 @@ PythonInteger::CreateStructuredSignedInteger() const {
// PythonBoolean
-PythonBoolean::PythonBoolean(bool value) {
- SetValue(value);
-}
+PythonBoolean::PythonBoolean(bool value) { SetValue(value); }
bool PythonBoolean::Check(PyObject *py_obj) {
return py_obj ? PyBool_Check(py_obj) : false;
@@ -856,15 +854,15 @@ PythonObject PythonCallable::operator()() {
return PythonObject(PyRefType::Owned, PyObject_CallObject(m_py_obj, nullptr));
}
-PythonObject PythonCallable::
-operator()(std::initializer_list<PyObject *> args) {
+PythonObject
+PythonCallable::operator()(std::initializer_list<PyObject *> args) {
PythonTuple arg_tuple(args);
return PythonObject(PyRefType::Owned,
PyObject_CallObject(m_py_obj, arg_tuple.get()));
}
-PythonObject PythonCallable::
-operator()(std::initializer_list<PythonObject> args) {
+PythonObject
+PythonCallable::operator()(std::initializer_list<PythonObject> args) {
PythonTuple arg_tuple(args);
return PythonObject(PyRefType::Owned,
PyObject_CallObject(m_py_obj, arg_tuple.get()));
@@ -1424,8 +1422,7 @@ Error PythonScript::Init() {
auto builtins = PythonModule::BuiltinsModule();
if (Error error = globals.SetItem("__builtins__", builtins))
return error;
- PyObject *o =
- PyRun_String(script, Py_file_input, globals.get(), globals.get());
+ PyObject *o = RunString(script, Py_file_input, globals.get(), globals.get());
if (!o)
return exception();
Take<PythonObject>(o);
@@ -1469,11 +1466,35 @@ python::runStringMultiLine(const llvm::Twine &string,
const PythonDictionary &locals) {
if (!globals.IsValid() || !locals.IsValid())
return nullDeref();
- PyObject *result = PyRun_String(NullTerminated(string), Py_file_input,
- globals.get(), locals.get());
+ PyObject *result = RunString(NullTerminated(string), Py_file_input,
+ globals.get(), locals.get());
if (!result)
return exception();
return Take<PythonObject>(result);
}
+namespace lldb_private {
+namespace python {
+PyObject *RunString(const char *str, int start, PyObject *globals,
+ PyObject *locals) {
+ PyObject *code = nullptr;
+ PyObject *result = nullptr;
+ const char *filename = "<string>";
+
+ // Compile the string into a code object.
+ code = Py_CompileString(str, filename, start);
+ if (code == nullptr)
+ return nullptr;
+
+ // Execute the code object.
+ result = PyEval_EvalCode(code, globals, locals);
+
+ // Clean up the code object.
+ Py_DECREF(code);
+
+ return result;
+}
+} // namespace python
+} // namespace lldb_private
+
#endif
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
index 88c1bb7e729e7..6a5dd43096363 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -194,8 +194,8 @@ template <typename T, char F> struct PassthroughFormat {
};
template <> struct PythonFormat<char *> : PassthroughFormat<char *, 's'> {};
-template <> struct PythonFormat<const char *> :
- PassthroughFormat<const char *, 's'> {};
+template <>
+struct PythonFormat<const char *> : PassthroughFormat<const char *, 's'> {};
template <> struct PythonFormat<char> : PassthroughFormat<char, 'b'> {};
template <>
struct PythonFormat<unsigned char> : PassthroughFormat<unsigned char, 'B'> {};
@@ -780,6 +780,9 @@ class StructuredPythonObject : public StructuredData::Generic {
operator=(const StructuredPythonObject &) = delete;
};
+PyObject *RunString(const char *str, int start, PyObject *globals,
+ PyObject *locals);
+
} // namespace python
} // namespace lldb_private
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
index 2dd92fc00fea1..0d4b04b7a1284 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -632,8 +632,8 @@ TEST_F(PythonDataObjectsTest, TestCallable) {
ASSERT_FALSE(error);
{
- PyObject *o = PyRun_String("lambda x : x", Py_eval_input, globals.get(),
- globals.get());
+ PyObject *o =
+ RunString("lambda x : x", Py_eval_input, globals.get(), globals.get());
ASSERT_FALSE(o == NULL);
auto lambda = Take<PythonCallable>(o);
auto arginfo = lambda.GetArgInfo();
@@ -642,8 +642,8 @@ TEST_F(PythonDataObjectsTest, TestCallable) {
}
{
- PyObject *o = PyRun_String("lambda x,y=0: x", Py_eval_input, globals.get(),
- globals.get());
+ PyObject *o = RunString("lambda x,y=0: x", Py_eval_input, globals.get(),
+ globals.get());
ASSERT_FALSE(o == NULL);
auto lambda = Take<PythonCallable>(o);
auto arginfo = lambda.GetArgInfo();
@@ -652,8 +652,8 @@ TEST_F(PythonDataObjectsTest, TestCallable) {
}
{
- PyObject *o = PyRun_String("lambda x,y=0, **kw: x", Py_eval_input,
- globals.get(), globals.get());
+ PyObject *o = RunString("lambda x,y=0, **kw: x", Py_eval_input,
+ globals.get(), globals.get());
ASSERT_FALSE(o == NULL);
auto lambda = Take<PythonCallable>(o);
auto arginfo = lambda.GetArgInfo();
@@ -662,8 +662,8 @@ TEST_F(PythonDataObjectsTest, TestCallable) {
}
{
- PyObject *o = PyRun_String("lambda x,y,*a: x", Py_eval_input, globals.get(),
- globals.get());
+ PyObject *o = RunString("lambda x,y,*a: x", Py_eval_input, globals.get(),
+ globals.get());
ASSERT_FALSE(o == NULL);
auto lambda = Take<PythonCallable>(o);
auto arginfo = lambda.GetArgInfo();
@@ -673,8 +673,8 @@ TEST_F(PythonDataObjectsTest, TestCallable) {
}
{
- PyObject *o = PyRun_String("lambda x,y,*a,**kw: x", Py_eval_input,
- globals.get(), globals.get());
+ PyObject *o = RunString("lambda x,y,*a,**kw: x", Py_eval_input,
+ globals.get(), globals.get());
ASSERT_FALSE(o == NULL);
auto lambda = Take<PythonCallable>(o);
auto arginfo = lambda.GetArgInfo();
@@ -713,7 +713,7 @@ class NewStyle(object):
)";
PyObject *o =
- PyRun_String(script, Py_file_input, globals.get(), globals.get());
+ RunString(script, Py_file_input, globals.get(), globals.get());
ASSERT_FALSE(o == NULL);
Take<PythonObject>(o);
|
JDevlieghere
force-pushed
the
reimplement-PyRun_String
branch
from
8fe60dd to
80affa3
Compare
Reimplement PyRun_String using Py_CompileString and PyEval_EvalCode, which are part of the stable C API. Part of llvm#151617
JDevlieghere
force-pushed
the
reimplement-PyRun_String
branch
from
80affa3 to
cb8d0d2
Compare
bulbazord
reviewed
Aug 1, 2025
| if (!result) | ||
| return exception(); | ||
| return Take<PythonObject>(result); | ||
| } | ||
|
|
||
| namespace lldb_private { | ||
| namespace python { |
There was a problem hiding this comment.
This doesn't need to be wrapped in namespaces?
There was a problem hiding this comment.
I believe it does. AFAIK you cannot implement a function declared in a namespace without explicitly qualifying it (i.e. the using namespace isn't enough). The compiler seems to agree with me.
There was a problem hiding this comment.
PyObject *lldb_private::python::RunString(...) { ?
There was a problem hiding this comment.
Do you think that's better? Note that I have more functions that'll go into this namespace.
There was a problem hiding this comment.
I don't think it's better or worse, but it is more consistent with how we've done class methods. I'll leave it up to you.
There was a problem hiding this comment.
I prefer the current approach. Another alternative would be to do it for the whole file.
There was a problem hiding this comment.
Opening the namespace halfway through the file looks weird.
In most of other places we use the syntax suggested by Alex. We also have some files which place the entire contents of the file inside the namespace. I think that looks less weird, and it is a style preferred by some style guides. However, I would argue that is not consistent with the llvm "make namespaces as small as possible" rule.
There was a problem hiding this comment.
I thought that rule was primarily aimed at anonymous namespaces, but I guess it applies here too. Regardless, it would be inconsistent which is why I didn't change the whole file. Seems like I'm the only one that prefers the namespaces so I'll change it to use the fully qualified names.
bulbazord
approved these changes
Aug 1, 2025
| if (!result) | ||
| return exception(); | ||
| return Take<PythonObject>(result); | ||
| } | ||
|
|
||
| namespace lldb_private { | ||
| namespace python { |
There was a problem hiding this comment.
I don't think it's better or worse, but it is more consistent with how we've done class methods. I'll leave it up to you.
JDevlieghere
added a commit
that referenced
this pull request
Aug 4, 2025
In #151761, both Alex and Pavel prefer to use a fully qualified name instead of opening a namespace. This PR addresses that post-commit feedback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reimplement
PyRun_StringusingPy_CompileStringandPyEval_EvalCode, which are part of the stable C API.Part of #151617