From 7375212172951d2fc283c81d03c1a8588c3280c6 Mon Sep 17 00:00:00 2001 From: Lawrence D'Anna Date: Tue, 21 Apr 2020 16:53:47 -0700 Subject: [PATCH] get rid of PythonInteger::GetInteger() Summary: One small step in my long running quest to improve python exception handling in LLDB. Replace GetInteger() which just returns an int with As and friends, which return Expected types that can track python exceptions Reviewers: labath, jasonmolenda, JDevlieghere, vadimcn Reviewed By: labath Subscribers: lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D78462 --- lldb/bindings/python/python-typemaps.swig | 36 +++------ lldb/bindings/python/python-wrapper.swig | 43 +++++------ .../Python/PythonDataObjects.cpp | 40 +++++----- .../Python/PythonDataObjects.h | 27 ++++++- .../Python/ScriptInterpreterPython.cpp | 13 +--- .../Python/PythonDataObjectsTests.cpp | 74 +++++++++---------- 6 files changed, 115 insertions(+), 118 deletions(-) diff --git a/lldb/bindings/python/python-typemaps.swig b/lldb/bindings/python/python-typemaps.swig index 46dcaf611a4f..c08aeab71f78 100644 --- a/lldb/bindings/python/python-typemaps.swig +++ b/lldb/bindings/python/python-typemaps.swig @@ -59,37 +59,25 @@ $result = list.release(); } - %typemap(in) lldb::tid_t { - if (PythonInteger::Check($input)) - { - PythonInteger py_int(PyRefType::Borrowed, $input); - $1 = static_cast(py_int.GetInteger()); - } - else - { - PyErr_SetString(PyExc_ValueError, "Expecting an integer"); + PythonObject obj = Retain($input); + lldb::tid_t value = unwrapOrSetPythonException(As(obj)); + if (PyErr_Occurred()) return nullptr; - } + $1 = value; } %typemap(in) lldb::StateType { - if (PythonInteger::Check($input)) - { - PythonInteger py_int(PyRefType::Borrowed, $input); - int64_t state_type_value = py_int.GetInteger() ; - - if (state_type_value > lldb::StateType::kLastStateType) { - PyErr_SetString(PyExc_ValueError, "Not a valid StateType value"); - return nullptr; - } - $1 = static_cast(state_type_value); - } - else - { - PyErr_SetString(PyExc_ValueError, "Expecting an integer"); + PythonObject obj = Retain($input); + unsigned long long state_type_value = + unwrapOrSetPythonException(As(obj)); + if (PyErr_Occurred()) + return nullptr; + if (state_type_value > lldb::StateType::kLastStateType) { + PyErr_SetString(PyExc_ValueError, "Not a valid StateType value"); return nullptr; } + $1 = static_cast(state_type_value); } /* Typemap definitions to allow SWIG to properly handle char buffer. */ diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig index 3a63165cf58d..f9e89373fe25 100644 --- a/lldb/bindings/python/python-wrapper.swig +++ b/lldb/bindings/python/python-wrapper.swig @@ -444,6 +444,7 @@ LLDBSwigPythonCallBreakpointResolver if (PyErr_Occurred()) { PyErr_Print(); + PyErr_Clear(); return 0; } @@ -457,11 +458,13 @@ LLDBSwigPythonCallBreakpointResolver return 1; } - PythonInteger int_result = result.AsType(); - if (!int_result.IsAllocated()) - return 0; + long long ret_val = unwrapOrSetPythonException(As(result)); - unsigned int ret_val = int_result.GetInteger(); + if (PyErr_Occurred()) { + PyErr_Print(); + PyErr_Clear(); + return 0; + } return ret_val; } @@ -515,26 +518,17 @@ LLDBSwigPython_CalculateNumChildren return 0; } - PythonObject result; - + size_t ret_val; if (arg_info.get().max_positional_args < 1) - result = pfunc(); + ret_val = unwrapOrSetPythonException(As(pfunc.Call())); else - result = pfunc(PythonInteger(max)); + ret_val = unwrapOrSetPythonException(As(pfunc.Call(PythonInteger(max)))); - if (!result.IsAllocated()) - return 0; - - PythonInteger int_result = result.AsType(); - if (!int_result.IsAllocated()) - return 0; - - size_t ret_val = int_result.GetInteger(); - - if (PyErr_Occurred()) //FIXME use Expected to catch python exceptions + if (PyErr_Occurred()) { PyErr_Print(); PyErr_Clear(); + return 0; } if (arg_info.get().max_positional_args < 1) @@ -588,16 +582,15 @@ LLDBSwigPython_GetIndexOfChildWithName if (!pfunc.IsAllocated()) return UINT32_MAX; - PythonObject result = pfunc(PythonString(child_name)); + llvm::Expected result = pfunc.Call(PythonString(child_name)); - if (!result.IsAllocated()) + long long retval = unwrapOrSetPythonException(As(std::move(result))); + + if (PyErr_Occurred()) { + PyErr_Clear(); // FIXME print this? do something else return UINT32_MAX; + } - PythonInteger int_result = result.AsType(); - if (!int_result.IsAllocated()) - return UINT32_MAX; - - int64_t retval = int_result.GetInteger(); if (retval >= 0) return (uint32_t)retval; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index 40ed22aceebf..3f00f671d88d 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -44,7 +44,15 @@ template <> Expected python::As(Expected &&obj) { if (!obj) return obj.takeError(); - return obj.get().AsLongLong(); + return obj->AsLongLong(); +} + +template <> +Expected +python::As(Expected &&obj) { + if (!obj) + return obj.takeError(); + return obj->AsUnsignedLongLong(); } template <> @@ -463,32 +471,22 @@ void PythonInteger::Convert(PyRefType &type, PyObject *&py_obj) { #endif } -int64_t PythonInteger::GetInteger() const { - if (m_py_obj) { - assert(PyLong_Check(m_py_obj) && - "PythonInteger::GetInteger has a PyObject that isn't a PyLong"); - - int overflow = 0; - int64_t result = PyLong_AsLongLongAndOverflow(m_py_obj, &overflow); - if (overflow != 0) { - // We got an integer that overflows, like 18446744072853913392L we can't - // use PyLong_AsLongLong() as it will return 0xffffffffffffffff. If we - // use the unsigned long long it will work as expected. - const uint64_t uval = PyLong_AsUnsignedLongLong(m_py_obj); - result = static_cast(uval); - } - return result; - } - return UINT64_MAX; -} - void PythonInteger::SetInteger(int64_t value) { *this = Take(PyLong_FromLongLong(value)); } StructuredData::IntegerSP PythonInteger::CreateStructuredInteger() const { StructuredData::IntegerSP result(new StructuredData::Integer); - result->SetValue(GetInteger()); + // FIXME this is really not ideal. Errors are silently converted to 0 + // and overflows are silently wrapped. But we'd need larger changes + // to StructuredData to fix it, so that's how it is for now. + llvm::Expected value = AsModuloUnsignedLongLong(); + if (!value) { + llvm::consumeError(value.takeError()); + result->SetValue(0); + } else { + result->SetValue(value.get()); + } return result; } diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h index 16896803e136..b09f42e0d540 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -370,6 +370,27 @@ public: return r; } + llvm::Expected AsUnsignedLongLong() { + if (!m_py_obj) + return nullDeref(); + assert(!PyErr_Occurred()); + long long r = PyLong_AsUnsignedLongLong(m_py_obj); + if (PyErr_Occurred()) + return exception(); + return r; + } + + llvm::Expected AsModuloUnsignedLongLong() const { + // wraps on overflow, instead of raising an error. + if (!m_py_obj) + return nullDeref(); + assert(!PyErr_Occurred()); + unsigned long long r = PyLong_AsUnsignedLongLongMask(m_py_obj); + if (PyErr_Occurred()) + return exception(); + return r; + } + llvm::Expected IsInstance(const PythonObject &cls) { if (!m_py_obj || !cls.IsValid()) return nullDeref(); @@ -399,6 +420,10 @@ template <> llvm::Expected As(llvm::Expected &&obj); template <> llvm::Expected As(llvm::Expected &&obj); +template <> +llvm::Expected +As(llvm::Expected &&obj); + template <> llvm::Expected As(llvm::Expected &&obj); @@ -491,8 +516,6 @@ public: static bool Check(PyObject *py_obj); static void Convert(PyRefType &type, PyObject *&py_obj); - int64_t GetInteger() const; - void SetInteger(int64_t value); StructuredData::IntegerSP CreateStructuredInteger() const; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp index c53b3bd0fb65..6f266772624a 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp @@ -3150,20 +3150,15 @@ uint32_t ScriptInterpreterPythonImpl::GetFlagsForCommandObject( if (PyErr_Occurred()) PyErr_Clear(); - // right now we know this function exists and is callable.. - PythonObject py_return( - PyRefType::Owned, - PyObject_CallMethod(implementor.get(), callee_name, nullptr)); + long long py_return = unwrapOrSetPythonException( + As(implementor.CallMethod(callee_name))); // if it fails, print the error but otherwise go on if (PyErr_Occurred()) { PyErr_Print(); PyErr_Clear(); - } - - if (py_return.IsAllocated() && PythonInteger::Check(py_return.get())) { - PythonInteger int_value(PyRefType::Borrowed, py_return.get()); - result = int_value.GetInteger(); + } else { + result = py_return; } return result; diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp index fe3b423b4842..75a1f5e67d32 100644 --- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp +++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp @@ -123,13 +123,11 @@ TEST_F(PythonDataObjectsTest, TestInstanceNameResolutionNoDot) { EXPECT_TRUE(major_version_field.IsAllocated()); EXPECT_TRUE(minor_version_field.IsAllocated()); - PythonInteger major_version_value = - major_version_field.AsType(); - PythonInteger minor_version_value = - minor_version_field.AsType(); + auto major_version_value = As(major_version_field); + auto minor_version_value = As(minor_version_field); - EXPECT_EQ(PY_MAJOR_VERSION, major_version_value.GetInteger()); - EXPECT_EQ(PY_MINOR_VERSION, minor_version_value.GetInteger()); + EXPECT_THAT_EXPECTED(major_version_value, llvm::HasValue(PY_MAJOR_VERSION)); + EXPECT_THAT_EXPECTED(minor_version_value, llvm::HasValue(PY_MINOR_VERSION)); } TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionWithDot) { @@ -137,16 +135,14 @@ TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionWithDot) { EXPECT_TRUE(sys_path.IsAllocated()); EXPECT_TRUE(PythonList::Check(sys_path.get())); - PythonInteger version_major = - m_main_module.ResolveName("sys.version_info.major") - .AsType(); - PythonInteger version_minor = - m_main_module.ResolveName("sys.version_info.minor") - .AsType(); - EXPECT_TRUE(version_major.IsAllocated()); - EXPECT_TRUE(version_minor.IsAllocated()); - EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger()); - EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger()); + auto version_major = + As(m_main_module.ResolveName("sys.version_info.major")); + + auto version_minor = + As(m_main_module.ResolveName("sys.version_info.minor")); + + EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION)); + EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION)); } TEST_F(PythonDataObjectsTest, TestDictionaryResolutionWithDot) { @@ -155,14 +151,14 @@ TEST_F(PythonDataObjectsTest, TestDictionaryResolutionWithDot) { dict.SetItemForKey(PythonString("sys"), m_sys_module); // Now use that dictionary to resolve `sys.version_info.major` - PythonInteger version_major = - PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict) - .AsType(); - PythonInteger version_minor = - PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict) - .AsType(); - EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger()); - EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger()); + auto version_major = As( + PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict)); + + auto version_minor = As( + PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict)); + + EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION)); + EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION)); } TEST_F(PythonDataObjectsTest, TestPythonInteger) { @@ -176,7 +172,8 @@ TEST_F(PythonDataObjectsTest, TestPythonInteger) { PythonInteger python_int(PyRefType::Owned, py_int); EXPECT_EQ(PyObjectType::Integer, python_int.GetObjectType()); - EXPECT_EQ(12, python_int.GetInteger()); + auto python_int_value = As(python_int); + EXPECT_THAT_EXPECTED(python_int_value, llvm::HasValue(12)); #endif // Verify that `PythonInteger` works correctly when given a PyLong object. @@ -187,12 +184,14 @@ TEST_F(PythonDataObjectsTest, TestPythonInteger) { // Verify that you can reset the value and that it is reflected properly. python_long.SetInteger(40); - EXPECT_EQ(40, python_long.GetInteger()); + auto e = As(python_long); + EXPECT_THAT_EXPECTED(e, llvm::HasValue(40)); // Test that creating a `PythonInteger` object works correctly with the // int constructor. PythonInteger constructed_int(7); - EXPECT_EQ(7, constructed_int.GetInteger()); + auto value = As(constructed_int); + EXPECT_THAT_EXPECTED(value, llvm::HasValue(7)); } TEST_F(PythonDataObjectsTest, TestPythonBoolean) { @@ -339,7 +338,8 @@ TEST_F(PythonDataObjectsTest, TestPythonListValueEquality) { PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get()); PythonString chk_str(PyRefType::Borrowed, chk_value2.get()); - EXPECT_EQ(long_value0, chk_int.GetInteger()); + auto chkint = As(chk_value1); + ASSERT_THAT_EXPECTED(chkint, llvm::HasValue(long_value0)); EXPECT_EQ(string_value1, chk_str.GetString()); } @@ -367,7 +367,8 @@ TEST_F(PythonDataObjectsTest, TestPythonListManipulation) { PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get()); PythonString chk_str(PyRefType::Borrowed, chk_value2.get()); - EXPECT_EQ(long_value0, chk_int.GetInteger()); + auto e = As(chk_int); + EXPECT_THAT_EXPECTED(e, llvm::HasValue(long_value0)); EXPECT_EQ(string_value1, chk_str.GetString()); } @@ -487,10 +488,10 @@ TEST_F(PythonDataObjectsTest, TestPythonDictionaryValueEquality) { EXPECT_TRUE(PythonInteger::Check(chk_value1.get())); EXPECT_TRUE(PythonString::Check(chk_value2.get())); - PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get()); PythonString chk_str(PyRefType::Borrowed, chk_value2.get()); + auto chkint = As(chk_value1); - EXPECT_EQ(value_0, chk_int.GetInteger()); + EXPECT_THAT_EXPECTED(chkint, llvm::HasValue(value_0)); EXPECT_EQ(value_1, chk_str.GetString()); } @@ -524,10 +525,10 @@ TEST_F(PythonDataObjectsTest, TestPythonDictionaryManipulation) { EXPECT_TRUE(PythonInteger::Check(chk_value1.get())); EXPECT_TRUE(PythonString::Check(chk_value2.get())); - PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get()); + auto chkint = As(chk_value1); PythonString chk_str(PyRefType::Borrowed, chk_value2.get()); - EXPECT_EQ(value_0, chk_int.GetInteger()); + EXPECT_THAT_EXPECTED(chkint, llvm::HasValue(value_0)); EXPECT_EQ(value_1, chk_str.GetString()); } @@ -594,10 +595,9 @@ TEST_F(PythonDataObjectsTest, TestObjectAttributes) { EXPECT_TRUE(py_int.HasAttribute("numerator")); EXPECT_FALSE(py_int.HasAttribute("this_should_not_exist")); - PythonInteger numerator_attr = - py_int.GetAttributeValue("numerator").AsType(); - EXPECT_TRUE(numerator_attr.IsAllocated()); - EXPECT_EQ(42, numerator_attr.GetInteger()); + auto numerator_attr = As(py_int.GetAttributeValue("numerator")); + + EXPECT_THAT_EXPECTED(numerator_attr, llvm::HasValue(42)); } TEST_F(PythonDataObjectsTest, TestExtractingUInt64ThroughStructuredData) {