bpo-26868: Fix example usage of PyModule_AddObject. #15725

brandtbucher · 2019-09-07T15:52:10Z

Unlike other APIs that steal references, PyModule_AddObject only does it on success. However, most calls throughout the codebase don't manually Py_DECREF on failure as required by the implementation, which leads contributors to believe that it is not necessary. This PR fixes example usage and adds a note on this behavior in the documentation only.

There are over 200 bad calls throughout the codebase that are not addressed by this patch. Because there are so many bugs, fixing them all will likely require more care and likely several PRs (or a possible deprecation period and behavior change... it appears incorrect usage is actually more common than correct usage). I'm willing to do the work, but some direction on this would be nice.

https://bugs.python.org/issue26868

brandtbucher · 2019-09-07T15:57:07Z

@serhiy-storchaka

Doc/c-api/module.rst

Doc/extending/extending.rst

Doc/includes/sublist.c

brandtbucher · 2019-09-09T17:55:23Z

Thanks for the feedback @serhiy-storchaka! Changes applied, tests are passing.

serhiy-storchaka · 2019-09-10T08:37:14Z

Doc/extending/extending.rst

-       Py_INCREF(SpamError);
-       PyModule_AddObject(m, "error", SpamError);
+       Py_XINCREF(SpamError);
+       if (PyModule_AddObject(m, "error", SpamError)) {


I suggest to add also < 0 in all other checks for PyModule_AddObject() result.

Is this an agreed-upon style? I had always thought it was a matter of taste.

In either case, I guess all of these files should at least be consistent...

Add the extra test with < 0

ghost · 2019-09-11T03:53:17Z

There are three sytles currently:

if (PyModule_AddObject())
if (PyModule_AddObject() < 0)
if (PyModule_AddObject() == -1)

After some thinking, I think the second one is not bad.

Usually APIs use a negative return number to indicate failure.
Even the reader is not familiar with Python code, if he/she sees < 0, may guess that it's likely to detect failure.
So style 2 is a bit better than style 1.

The third style may produce a very slightly longer machine code:

; 19   :     if (PyModule_AddObject())
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	85 c0		 test	 eax, eax
  0000b	74 07		 je	 SHORT $LN2@main

; 21   :     if (PyModule_AddObject() < 0)
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	85 c0		 test	 eax, eax
  0000b	7d 07		 jge	 SHORT $LN2@main

; 23   :     if (PyModule_AddObject() == -1)
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	83 f8 ff	 cmp	 eax, -1
  0000c	75 07		 jne	 SHORT $LN2@main

And < 0 is likely to indicate that all failures are covered, not a specific failure.
So style 2 is a bit better than style 3.

There are reasons, although the reasons are weak. 😂

matrixise

Hi, Thank you for your contribution but could you add the small requested changes.

Thank you

matrixise · 2019-09-12T08:16:27Z

Doc/extending/extending.rst

@@ -1261,8 +1267,12 @@ function must take care of initializing the C API pointer array::
       /* Create a Capsule containing the API pointer array's address */
       c_api_object = PyCapsule_New((void *)PySpam_API, "spam._C_API", NULL);

-       if (c_api_object != NULL)
-           PyModule_AddObject(m, "_C_API", c_api_object);
+       if (PyModule_AddObject(m, "_C_API", c_api_object)) {


ditto, change the example with if (PyModule_AddObject(m, "_C_API", c_api_object) < 0)

matrixise · 2019-09-12T08:16:44Z

Doc/extending/newtypes_tutorial.rst

@@ -179,7 +179,12 @@ This initializes the :class:`Custom` type, filling in a number of members
 to the appropriate default values, including :attr:`ob_type` that we initially
 set to *NULL*. ::

-   PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+   Py_INCREF(&CustomType);
+   if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:16:53Z

Doc/extending/newtypes_tutorial.rst

@@ -864,7 +869,12 @@ function::
           return NULL;

       Py_INCREF(&SubListType);
-       PyModule_AddObject(m, "SubList", (PyObject *) &SubListType);
+       if (PyModule_AddObject(m, "SubList", (PyObject *) &SubListType)) {


matrixise · 2019-09-12T08:16:59Z

Doc/includes/custom.c

@@ -35,6 +35,11 @@ PyInit_custom(void)
        return NULL;

    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:06Z

Doc/includes/custom2.c

@@ -128,6 +128,11 @@ PyInit_custom2(void)
        return NULL;

    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:13Z

Doc/includes/custom3.c

@@ -179,6 +179,11 @@ PyInit_custom3(void)
        return NULL;

    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:19Z

Doc/includes/custom4.c

@@ -193,6 +193,11 @@ PyInit_custom4(void)
        return NULL;

    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:26Z

Doc/includes/sublist.c

@@ -59,6 +59,11 @@ PyInit_sublist(void)
        return NULL;

    Py_INCREF(&SubListType);
-    PyModule_AddObject(m, "SubList", (PyObject *) &SubListType);
+    if (PyModule_AddObject(m, "SubList", (PyObject *) &SubListType)) {


matrixise

Thank you for your contribution, I have fixed the remarks with < 0 in the examples. I am going to merge it.

bedevere-bot · 2019-09-12T12:11:23Z

@matrixise: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2019-09-12T12:11:23Z

Thanks @brandtbucher for the PR, and @matrixise for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

bedevere-bot · 2019-09-12T12:11:33Z

GH-16037 is a backport of this pull request to the 3.8 branch.

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

bedevere-bot · 2019-09-12T12:11:40Z

GH-16038 is a backport of this pull request to the 3.7 branch.

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

brandtbucher · 2019-09-12T13:25:56Z

Thanks @matrixise!

brandtbucher added 4 commits September 7, 2019 07:53

Add a note to the PyModule_AddObject docs.

90ae263

Correct example usages of PyModule_AddObject.

4b08887

Whitespace.

aef02b8

Clean up wording.

474a266

the-knights-who-say-ni added the CLA signed label Sep 7, 2019

bedevere-bot added the awaiting review label Sep 7, 2019

brandtbucher added needs backport to 3.7 docs Documentation in the Doc dir labels Sep 7, 2019

📜🤖 Added by blurb_it.

a005823

serhiy-storchaka reviewed Sep 7, 2019

View reviewed changes

Yhg1s added awaiting changes and removed awaiting review labels Sep 9, 2019

brandtbucher mentioned this pull request Sep 9, 2019

bpo-38037: Fix reference counters in signal module #15701

Closed

First code review.

bb345d9

serhiy-storchaka reviewed Sep 10, 2019

View reviewed changes

matrixise requested changes Sep 12, 2019

View reviewed changes

matrixise self-assigned this Sep 12, 2019

Add < 0 in the tests with PyModule_AddObject

37b6b37

matrixise approved these changes Sep 12, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 12, 2019

matrixise approved these changes Sep 12, 2019

View reviewed changes

matrixise merged commit 224b8aa into python:master Sep 12, 2019

bedevere-bot removed the awaiting merge label Sep 12, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 12, 2019

bedevere-bot removed the needs backport to 3.7 label Sep 12, 2019

brandtbucher deleted the safe-add-object branch September 12, 2019 13:26

berkerpeksag mentioned this pull request Oct 29, 2022

Document PyModule_AddObject's behavior on error #71055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-26868: Fix example usage of PyModule_AddObject. #15725

bpo-26868: Fix example usage of PyModule_AddObject. #15725

brandtbucher commented Sep 7, 2019 •

edited by bedevere-bot

Loading

brandtbucher commented Sep 7, 2019

brandtbucher commented Sep 9, 2019

serhiy-storchaka Sep 10, 2019

brandtbucher Sep 10, 2019

matrixise Sep 11, 2019

ghost commented Sep 11, 2019

matrixise left a comment

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise Sep 12, 2019

matrixise left a comment

bedevere-bot commented Sep 12, 2019

miss-islington commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

brandtbucher commented Sep 12, 2019

bpo-26868: Fix example usage of PyModule_AddObject. #15725

bpo-26868: Fix example usage of PyModule_AddObject. #15725

Conversation

brandtbucher commented Sep 7, 2019 • edited by bedevere-bot Loading

brandtbucher commented Sep 7, 2019

brandtbucher commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 11, 2019

matrixise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixise left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 12, 2019

miss-islington commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

brandtbucher commented Sep 12, 2019

brandtbucher commented Sep 7, 2019 •

edited by bedevere-bot

Loading