From 52712d3ff7a2f7bcf737996d6ab59ef2cc29c20d Mon Sep 17 00:00:00 2001 From: Lawrence D'Anna Date: Fri, 8 May 2020 10:56:30 -0700 Subject: [PATCH] Re-land "get rid of PythonInteger::GetInteger()" This was reverted due to a python2-specific bug. Re-landing with a fix for python2. 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, omjavaid Reviewed By: labath, omjavaid Subscribers: omjavaid, 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 | 89 ++++++++++++++----- .../Python/PythonDataObjects.h | 21 +++-- .../Python/ScriptInterpreterPython.cpp | 13 +-- .../Python/PythonDataObjectsTests.cpp | 74 +++++++-------- 6 files changed, 149 insertions(+), 127 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..6f040fdef09b 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 <> @@ -61,6 +69,55 @@ Expected python::As(Expected &&obj) { return std::string(utf8.get()); } +Expected PythonObject::AsLongLong() const { + if (!m_py_obj) + return nullDeref(); +#if PY_MAJOR_VERSION < 3 + if (!PyLong_Check(m_py_obj)) { + PythonInteger i(PyRefType::Borrowed, m_py_obj); + return i.AsLongLong(); + } +#endif + assert(!PyErr_Occurred()); + long long r = PyLong_AsLongLong(m_py_obj); + if (PyErr_Occurred()) + return exception(); + return r; +} + +Expected PythonObject::AsUnsignedLongLong() const { + if (!m_py_obj) + return nullDeref(); +#if PY_MAJOR_VERSION < 3 + if (!PyLong_Check(m_py_obj)) { + PythonInteger i(PyRefType::Borrowed, m_py_obj); + return i.AsUnsignedLongLong(); + } +#endif + assert(!PyErr_Occurred()); + long long r = PyLong_AsUnsignedLongLong(m_py_obj); + if (PyErr_Occurred()) + return exception(); + return r; +} + +// wraps on overflow, instead of raising an error. +Expected PythonObject::AsModuloUnsignedLongLong() const { + if (!m_py_obj) + return nullDeref(); +#if PY_MAJOR_VERSION < 3 + if (!PyLong_Check(m_py_obj)) { + PythonInteger i(PyRefType::Borrowed, m_py_obj); + return i.AsModuloUnsignedLongLong(); + } +#endif + assert(!PyErr_Occurred()); + unsigned long long r = PyLong_AsUnsignedLongLongMask(m_py_obj); + if (PyErr_Occurred()) + return exception(); + return r; +} + void StructuredPythonObject::Serialize(llvm::json::OStream &s) const { s.value(llvm::formatv("Python Obj: {0:X}", GetValue()).str()); } @@ -463,32 +520,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..ba127eae10a8 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -360,15 +360,12 @@ public: return !!r; } - llvm::Expected AsLongLong() { - if (!m_py_obj) - return nullDeref(); - assert(!PyErr_Occurred()); - long long r = PyLong_AsLongLong(m_py_obj); - if (PyErr_Occurred()) - return exception(); - return r; - } + llvm::Expected AsLongLong() const; + + llvm::Expected AsUnsignedLongLong() const; + + // wraps on overflow, instead of raising an error. + llvm::Expected AsModuloUnsignedLongLong() const; llvm::Expected IsInstance(const PythonObject &cls) { if (!m_py_obj || !cls.IsValid()) @@ -399,6 +396,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 +492,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) {