[lldb] Update some uses of Python2 API in typemaps.

[jgorbe]

Summary

Python 3 doesn't have a distinction between PyInt and PyLong, it's all
PyLong now.

This also fixes a bug in SetNumberFromObject. This used to crash LLDB:

lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"

The problem happened in the PyInt path:

if (PyInt_Check(obj))
    number = static_cast<T>(PyInt_AsLong(obj));

when obj doesn't fit in a signed long, PyInt_AsLong would fail with
"OverflowError: Python int too large to convert to C long".

The existing long path does the right thing, as it will call
PyLong_AsUnsignedLongLong for uint64_t.

[rupprecht]

Can you add a regression test that invokes lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"? Just a simple shell test should suffice.

LGTM, but it'd be nice if someone else can look too. Adding some other people that have been here recently.

[bulbazord]

lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"

Just tried this, didn't crash LLDB but it does give me the OverflowError you mentioned.

Either way, this is probably okay? What do you think @mib?

[rupprecht]

In D146590#4211763, @bulbazord wrote:

lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"

Just tried this, didn't crash LLDB but it does give me the OverflowError you mentioned.

Here's what the crash looks like for me:

$ bin/lldb -o "script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])"
(lldb) script data=lldb.SBData(); data.SetDataFromUInt64Array([2**63])
free(): double free detected in tcache 2
LLDB diagnostics will be written to /tmp/diagnostics-98b663
Please include the directory content when filing a bug report
Aborted

[mib]

Yep, LGTM!

[jgorbe]

Modified one of the existing test cases for SBData.SetDataFromUInt64Array to add a 2**63 to actually exercise the uint64 range.

[jgorbe]

I found the actual reason for the crash I was talking about. This patch only addressed the incorrect OverflowError, but the crash comes from a double free (as @rupprecht mentioned) in the error handling logic. The error path here does both free($1); and SWIG_fail;. The same goes for another error check a few lines below that. In the generated code, the SWIG_fail macro is expanded to goto fail and the fail label also frees the same memory buffer.

I believe (but I don't have any experience with SWIG typemaps so this is an educated guess) that the call to free in the error path comes from the %typemap(freearg) immediately after that one. So if freearg already takes care of it, the error handling logic in %typemap(in) should just call SWIG_fail. Does that sound correct?

[labath]

In D146590#4220388, @jgorbe wrote:

I believe (but I don't have any experience with SWIG typemaps so this is an educated guess) that the call to free in the error path comes from the %typemap(freearg) immediately after that one. So if freearg already takes care of it, the error handling logic in %typemap(in) should just call SWIG_fail. Does that sound correct?

Makes sense to me.

[bulbazord]

In D146590#4220388, @jgorbe wrote:

I found the actual reason for the crash I was talking about. This patch only addressed the incorrect OverflowError, but the crash comes from a double free (as @rupprecht mentioned) in the error handling logic. The error path here does both free($1); and SWIG_fail;. The same goes for another error check a few lines below that. In the generated code, the SWIG_fail macro is expanded to goto fail and the fail label also frees the same memory buffer.

I believe (but I don't have any experience with SWIG typemaps so this is an educated guess) that the call to free in the error path comes from the %typemap(freearg) immediately after that one. So if freearg already takes care of it, the error handling logic in %typemap(in) should just call SWIG_fail. Does that sound correct?

Yea, after reading the SWIG documentation, this diagnosis looks correct. If you'd like to fix this feel free to upload a patch and list myself and @mib as reviewers. Otherwise let me know and I can take care of it. Thanks for doing that investigation!

[jgorbe]

In D146590#4225500, @bulbazord wrote:

Yea, after reading the SWIG documentation, this diagnosis looks correct. If you'd like to fix this feel free to upload a patch and list myself and @mib as reviewers. Otherwise let me know and I can take care of it. Thanks for doing that investigation!

Thanks! Just sent you all https://reviews.llvm.org/D147007

lldb/bindings/python/python-typemaps.swig

%typemap(in) lldb::StateType {
   }
   $1 = static_cast<lldb::StateType>(state_type_value);
 }
 
 /* Typemap definitions to allow SWIG to properly handle char buffer. */
 
 // typemap for a char buffer
 %typemap(in) (char *dst, size_t dst_len) {
-  if (!PyInt_Check($input)) {
+  if (!PyLong_Check($input)) {
     PyErr_SetString(PyExc_ValueError, "Expecting an integer");
     SWIG_fail;
   }
-  $2 = PyInt_AsLong($input);
+  $2 = PyLong_AsLong($input);
   if ($2 <= 0) {
     PyErr_SetString(PyExc_ValueError, "Positive integer expected");
     SWIG_fail;
   }
   $1 = (char *)malloc($2);
 }
 // SBProcess::ReadCStringFromMemory() uses a void*, but needs to be treated
 // as char data instead of byte data.

 }
 // SBProcess::ReadCStringFromMemory() uses a void*, but needs to be treated
 // as char data instead of byte data.
 %typemap(argout) (void *char_buf, size_t size) = (char *dst, size_t dst_len);
 
 
 // typemap for handling an snprintf-like API like SBThread::GetStopDescription.
 %typemap(in) (char *dst_or_null, size_t dst_len) {
-  if (!PyInt_Check($input)) {
+  if (!PyLong_Check($input)) {
     PyErr_SetString(PyExc_ValueError, "Expecting an integer");
     SWIG_fail;
   }
-  $2 = PyInt_AsLong($input);
+  $2 = PyLong_AsLong($input);
   if ($2 <= 0) {
     PyErr_SetString(PyExc_ValueError, "Positive integer expected");
     SWIG_fail;
   }
   $1 = (char *)malloc($2);
 }
 %typemap(argout) (char *dst_or_null, size_t dst_len) {
   Py_XDECREF($result); /* Blow away any previous result */
if (PythonString::Check($input)) {
     PyErr_SetString(PyExc_ValueError, "Expecting a buffer");
     SWIG_fail;
   }
 }
 
 // typemap for an incoming buffer
 // See also SBProcess::ReadMemory.
 %typemap(in) (void *buf, size_t size) {
-  if (PyInt_Check($input)) {
-    $2 = PyInt_AsLong($input);
-  } else if (PyLong_Check($input)) {
+  if (PyLong_Check($input)) {
     $2 = PyLong_AsLong($input);
   } else {
     PyErr_SetString(PyExc_ValueError, "Expecting an integer or long object");
     SWIG_fail;
   }
   if ($2 <= 0) {
     PyErr_SetString(PyExc_ValueError, "Positive integer expected");
     SWIG_fail;
template <> int64_t PyLongAsT<int64_t>(PyObject *obj) {
   return static_cast<int64_t>(PyLong_AsLongLong(obj));
 }
 
 template <> int32_t PyLongAsT<int32_t>(PyObject *obj) {
   return static_cast<int32_t>(PyLong_AsLong(obj));
 }
 
 template <class T> bool SetNumberFromPyObject(T &number, PyObject *obj) {
-  if (PyInt_Check(obj))
-    number = static_cast<T>(PyInt_AsLong(obj));
-  else if (PyLong_Check(obj))
+  if (PyLong_Check(obj))
     number = PyLongAsT<T>(obj);
   else
     return false;
 
   return true;
 }
 
 template <> bool SetNumberFromPyObject<double>(double &number, PyObject *obj) {

 }
 
 %typemap(argout) (uint32_t *versions, uint32_t num_versions) {
   uint32_t count = result;
   if (count >= $2)
     count = $2;
   PyObject *list = PyList_New(count);
   for (uint32_t j = 0; j < count; j++) {
-    PyObject *item = PyInt_FromLong($1[j]);
+    PyObject *item = PyLong_FromLong($1[j]);
     int ok = PyList_SetItem(list, j, item);
     if (ok != 0) {
       $result = Py_None;
       break;
     }
   }
   $result = list;
 }

lldb/test/API/python_api/sbdata/TestSBData.py

def test_with_run_command(self):
         self.assertEqual(len(data2.uint8), len(hello_str))
         self.assert_data(data2.GetUnsignedInt8, 0, 104)
         self.assert_data(data2.GetUnsignedInt8, 1, 101)
         self.assert_data(data2.GetUnsignedInt8, 2, 108)
         self.assert_data(data2.GetUnsignedInt8, 3, 108)
         self.assert_data(data2.GetUnsignedInt8, 4, 111)
         self.assert_data(data2.GetUnsignedInt8, 5, 33)
 
-        data2.SetDataFromUInt64Array([1, 2, 3, 4, 5])
+        data2.SetDataFromUInt64Array([1, 2, 3, 4, 5, 2**63])
         self.assert_data(data2.GetUnsignedInt64, 0, 1)
         self.assert_data(data2.GetUnsignedInt64, 8, 2)
         self.assert_data(data2.GetUnsignedInt64, 16, 3)
         self.assert_data(data2.GetUnsignedInt64, 24, 4)
         self.assert_data(data2.GetUnsignedInt64, 32, 5)
+        self.assert_data(data2.GetUnsignedInt64, 40, 2**63)
 
         self.assertEqual(
             data2.uint64[0], 1,
             'read_data_helper failure: set data2[0] = 1')
         self.assertEqual(
             data2.uint64[1], 2,
             'read_data_helper failure: set data2[1] = 2')
         self.assertEqual(