[Lldb-commits] [lldb] 7375212 - get rid of PythonInteger::GetInteger()

Lawrence D'Anna via lldb-commits lldb-commits at lists.llvm.org
Tue Apr 21 17:17:45 PDT 2020


Author: Lawrence D'Anna
Date: 2020-04-21T16:55:51-07:00
New Revision: 7375212172951d2fc283c81d03c1a8588c3280c6

URL: https://github.com/llvm/llvm-project/commit/7375212172951d2fc283c81d03c1a8588c3280c6
DIFF: https://github.com/llvm/llvm-project/commit/7375212172951d2fc283c81d03c1a8588c3280c6.diff

LOG: 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<long long> 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

Added: 
    

Modified: 
    lldb/bindings/python/python-typemaps.swig
    lldb/bindings/python/python-wrapper.swig
    lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
    lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
    lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
    lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Removed: 
    


################################################################################
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<lldb::tid_t>(py_int.GetInteger());
-  }
-  else
-  {
-    PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+  PythonObject obj = Retain<PythonObject>($input);
+  lldb::tid_t value = unwrapOrSetPythonException(As<unsigned long long>(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<lldb::StateType>(state_type_value);
-  }
-  else
-  {
-    PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+  PythonObject obj = Retain<PythonObject>($input);
+  unsigned long long state_type_value =
+    unwrapOrSetPythonException(As<unsigned long long>(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<lldb::StateType>(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<PythonInteger>();
-    if (!int_result.IsAllocated())
-        return 0;
+    long long ret_val = unwrapOrSetPythonException(As<long long>(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<long long>(pfunc.Call()));
     else
-        result = pfunc(PythonInteger(max));
-
-    if (!result.IsAllocated())
-        return 0;
-
-    PythonInteger int_result = result.AsType<PythonInteger>();
-    if (!int_result.IsAllocated())
-        return 0;
-
-    size_t ret_val = int_result.GetInteger();
+        ret_val = unwrapOrSetPythonException(As<long long>(pfunc.Call(PythonInteger(max))));
 
-    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<PythonObject> result = pfunc.Call(PythonString(child_name));
 
-    if (!result.IsAllocated())
-        return UINT32_MAX;
+    long long retval = unwrapOrSetPythonException(As<long long>(std::move(result)));
 
-    PythonInteger int_result = result.AsType<PythonInteger>();
-    if (!int_result.IsAllocated())
+    if (PyErr_Occurred()) { 
+        PyErr_Clear(); // FIXME print this? do something else
         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<long long> python::As<long long>(Expected<PythonObject> &&obj) {
   if (!obj)
     return obj.takeError();
-  return obj.get().AsLongLong();
+  return obj->AsLongLong();
+}
+
+template <>
+Expected<unsigned long long>
+python::As<unsigned long long>(Expected<PythonObject> &&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<int64_t>(uval);
-    }
-    return result;
-  }
-  return UINT64_MAX;
-}
-
 void PythonInteger::SetInteger(int64_t value) {
   *this = Take<PythonInteger>(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<unsigned long long> 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 @@ class PythonObject {
     return r;
   }
 
+  llvm::Expected<long long> 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<unsigned long long> 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<bool> IsInstance(const PythonObject &cls) {
     if (!m_py_obj || !cls.IsValid())
       return nullDeref();
@@ -399,6 +420,10 @@ template <> llvm::Expected<bool> As<bool>(llvm::Expected<PythonObject> &&obj);
 template <>
 llvm::Expected<long long> As<long long>(llvm::Expected<PythonObject> &&obj);
 
+template <>
+llvm::Expected<unsigned long long>
+As<unsigned long long>(llvm::Expected<PythonObject> &&obj);
+
 template <>
 llvm::Expected<std::string> As<std::string>(llvm::Expected<PythonObject> &&obj);
 
@@ -491,8 +516,6 @@ class PythonInteger : public TypedPythonObject<PythonInteger> {
   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<long long>(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>();
-  PythonInteger minor_version_value =
-      minor_version_field.AsType<PythonInteger>();
+  auto major_version_value = As<long long>(major_version_field);
+  auto minor_version_value = As<long long>(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>();
-  PythonInteger version_minor =
-      m_main_module.ResolveName("sys.version_info.minor")
-          .AsType<PythonInteger>();
-  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<long long>(m_main_module.ResolveName("sys.version_info.major"));
+
+  auto version_minor =
+      As<long long>(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>();
-  PythonInteger version_minor =
-      PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict)
-          .AsType<PythonInteger>();
-  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
+  auto version_major = As<long long>(
+      PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict));
+
+  auto version_minor = As<long long>(
+      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<long long>(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<long long>(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<long long>(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<long long>(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<long long>(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<long long>(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<long long>(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<PythonInteger>();
-  EXPECT_TRUE(numerator_attr.IsAllocated());
-  EXPECT_EQ(42, numerator_attr.GetInteger());
+  auto numerator_attr = As<long long>(py_int.GetAttributeValue("numerator"));
+
+  EXPECT_THAT_EXPECTED(numerator_attr, llvm::HasValue(42));
 }
 
 TEST_F(PythonDataObjectsTest, TestExtractingUInt64ThroughStructuredData) {


        


More information about the lldb-commits mailing list