Skip to content

[lldb] Use PyBytes and PyByteArray in Python Data Objects unittest #82098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

JDevlieghere
Copy link
Member

Use a Python Bytes and ByteArray object instead of Integers for
TestOwnedReferences and TestBorrowedReferences. These two tests were
failing when building against Python 3.12 because these Integer objects
had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect
the Python runtime has adopted an optimization to decrease refcounting
traffic for these simple objects.

Use a Python Bytes and ByteArray object instead of Integers for
TestOwnedReferences and TestBorrowedReferences. These two tests were
failing when building against Python 3.12 because these Integer objects
had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect
the Python runtime has adopted an optimization to decrease refcounting
traffic for these simple objects.
@JDevlieghere JDevlieghere marked this pull request as ready for review February 17, 2024 07:39
@llvmbot llvmbot added the lldb label Feb 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2024

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Use a Python Bytes and ByteArray object instead of Integers for
TestOwnedReferences and TestBorrowedReferences. These two tests were
failing when building against Python 3.12 because these Integer objects
had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect
the Python runtime has adopted an optimization to decrease refcounting
traffic for these simple objects.


Full diff: https://github.com/llvm/llvm-project/pull/82098.diff

1 Files Affected:

  • (modified) lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp (+18-12)
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
index efb8f725f6739a..a4db4627f935b4 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -51,21 +51,24 @@ class PythonDataObjectsTest : public PythonTestSuite {
 
 TEST_F(PythonDataObjectsTest, TestOwnedReferences) {
   // After creating a new object, the refcount should be >= 1
-  PyObject *obj = PyLong_FromLong(3);
-  Py_ssize_t original_refcnt = obj->ob_refcnt;
+  PyObject *obj = PyBytes_FromString("foo");
+  Py_ssize_t original_refcnt = Py_REFCNT(obj);
   EXPECT_LE(1, original_refcnt);
 
   // If we take an owned reference, the refcount should be the same
-  PythonObject owned_long(PyRefType::Owned, obj);
-  EXPECT_EQ(original_refcnt, owned_long.get()->ob_refcnt);
+  PythonObject owned(PyRefType::Owned, obj);
+  Py_ssize_t owned_refcnt = Py_REFCNT(owned.get());
+  EXPECT_EQ(original_refcnt, owned_refcnt);
 
   // Take another reference and verify that the refcount increases by 1
-  PythonObject strong_ref(owned_long);
-  EXPECT_EQ(original_refcnt + 1, strong_ref.get()->ob_refcnt);
+  PythonObject strong_ref(owned);
+  Py_ssize_t strong_refcnt = Py_REFCNT(strong_ref.get());
+  EXPECT_EQ(original_refcnt + 1, strong_refcnt);
 
   // If we reset the first one, the refcount should be the original value.
-  owned_long.Reset();
-  EXPECT_EQ(original_refcnt, strong_ref.get()->ob_refcnt);
+  owned.Reset();
+  strong_refcnt = Py_REFCNT(strong_ref.get());
+  EXPECT_EQ(original_refcnt, strong_refcnt);
 }
 
 TEST_F(PythonDataObjectsTest, TestResetting) {
@@ -82,12 +85,15 @@ TEST_F(PythonDataObjectsTest, TestResetting) {
 }
 
 TEST_F(PythonDataObjectsTest, TestBorrowedReferences) {
-  PythonInteger long_value(PyRefType::Owned, PyLong_FromLong(3));
-  Py_ssize_t original_refcnt = long_value.get()->ob_refcnt;
+  PythonByteArray byte_value(PyRefType::Owned,
+                             PyByteArray_FromStringAndSize("foo", 3));
+  Py_ssize_t original_refcnt = Py_REFCNT(byte_value.get());
   EXPECT_LE(1, original_refcnt);
 
-  PythonInteger borrowed_long(PyRefType::Borrowed, long_value.get());
-  EXPECT_EQ(original_refcnt + 1, borrowed_long.get()->ob_refcnt);
+  PythonByteArray borrowed_byte(PyRefType::Borrowed, byte_value.get());
+  Py_ssize_t borrowed_refcnt = Py_REFCNT(borrowed_byte.get());
+
+  EXPECT_EQ(original_refcnt + 1, borrowed_refcnt);
 }
 
 TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionNoDot) {

@JDevlieghere JDevlieghere merged commit 27f2908 into llvm:main Feb 17, 2024
@JDevlieghere JDevlieghere deleted the PythonDataObjectsTest branch February 17, 2024 19:37
@hawkinsw
Copy link
Contributor

Use a Python Bytes and ByteArray object instead of Integers for TestOwnedReferences and TestBorrowedReferences. These two tests were failing when building against Python 3.12 because these Integer objects had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect the Python runtime has adopted an optimization to decrease refcounting traffic for these simple objects.

Perhaps related to ...

https://github.com/python/cpython/pull/30872/files#diff-1a6e70e2beeecad88840c67284ac4d54a36998029244771fcc820e801390726a

I was curious and investigated. Whether that is the exact change, it seems to confirm your hypothesis that there is special refcounting code for certain types of Python objects. Would you agree with that?

I hope that helps!

JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2024
…lvm#82098)

Use a Python Bytes and ByteArray object instead of Integers for
TestOwnedReferences and TestBorrowedReferences. These two tests were
failing when building against Python 3.12 because these Integer objects
had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect
the Python runtime has adopted an optimization to decrease refcounting
traffic for these simple objects.

(cherry picked from commit 27f2908)
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request May 24, 2024
…lvm#82098)

Use a Python Bytes and ByteArray object instead of Integers for
TestOwnedReferences and TestBorrowedReferences. These two tests were
failing when building against Python 3.12 because these Integer objects
had a refcount of 4294967296 (-1). I didn't dig into it, but I suspect
the Python runtime has adopted an optimization to decrease refcounting
traffic for these simple objects.

(cherry picked from commit 27f2908)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants