Proposal: C API Macro to decref and set to NULL
Often, in C type implementations, you want to set a PyObject * slot to NULL. Often this goes like: Py_XDECREF(self->foo); self->foo = NULL; However, if your type participates in GC and self->foo is something that might get traversed, this can lead to traversing self->foo after it has been DECREFed, but before it's set to NULL. The correct way to do this is something like: tmp = self->foo; self->foo = NULL; Py_XDECREF(tmp); I suggest that there should be a standard macro to automate this. Py_CLEAR(self->foo) This would be defined to be the same result as Py_XDECREF except that the argument will be set to NULL. Thoughts? If there are no objections I'll add the following definition to object.h, after the definition for Py_DECREF: #define Py_CLEAR(op) \ do { \ if (op) { \ PyObject *tmp = (op); \ (op) = NULL; \ Py_DECREF(tmp); \ } \ } while (0) and update the docs and the tutorial on creating types in C. Jim -- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
[Jim Fulton] ...
If there are no objections I'll add the following definition to object.h, after the definition for Py_DECREF:
#define Py_CLEAR(op) \ do { \ if (op) { \ PyObject *tmp = (op); \ (op) = NULL; \ Py_DECREF(tmp); \ } \ } while (0)
and update the docs and the tutorial on creating types in C.
+1. At least pystate.c can get rid of its functionally identical private ZAP macro then. BTW, re-invented at least as often is a VISIT macro for use in tp_traverse slots, like typeobject.c's (and several other files') #define VISIT(SLOT) \ if (SLOT) { \ err = visit((PyObject *)(SLOT), arg); \ if (err) \ return err; \ }
Tim Peters wrote:
[Jim Fulton] ...
...
BTW, re-invented at least as often is a VISIT macro for use in tp_traverse slots, like typeobject.c's (and several other files')
#define VISIT(SLOT) \ if (SLOT) { \ err = visit((PyObject *)(SLOT), arg); \ if (err) \ return err; \ }
First, I don't like this macro, based on my own experience writing macros that hide returns. :) Second, Aaargh! I missunderstood the use of the visit function passed to traverse. The source of my missunderstanding was 1) visit is an int function and I normally expect int functions to return a negative value to indicate an error and 2) The documentation for the traversal slot says: "If visit returns a non-zero value then an error has occurred and that value should be returned immediately." That is, the documentation says that the return value is an error indicator. I missed the bit about a non-zero return value and needing to return it. My bad. Last night, Tim explained to me that a non-zero return value is not an error indicator. In fact, the GC system doesn't really allow errors. Rather, a non-zero return value provides a way for a visit proc to short-circuit the traversal process. As far as Tim knows, this feature has never been used. All visit functions in the core always return 0. Alas, all my extensions that implement tp_traverse and the documentation I wrote on writing types is wrong and has to be changed. Sigh. Should we call WHUI (we haven't used it!) on this feature that has been around for 4 versions of Python, that complicates tp_traverse implementations and hasn't been used? It would be simpler if we said that the return value could be ignored. Jim -- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
Never mind. Tim pointed out to me that return values other than 0 for the visit proc are used. The documentation is still off I think, as it says that a non-zero value indicates an error, but I don't think that this is true. Jim Jim Fulton wrote:
Tim Peters wrote:
[Jim Fulton] ...
...
BTW, re-invented at least as often is a VISIT macro for use in tp_traverse slots, like typeobject.c's (and several other files')
#define VISIT(SLOT) \ if (SLOT) { \ err = visit((PyObject *)(SLOT), arg); \ if (err) \ return err; \ }
First, I don't like this macro, based on my own experience writing macros that hide returns. :)
Second, Aaargh! I missunderstood the use of the visit function passed to traverse. The source of my missunderstanding was
1) visit is an int function and I normally expect int functions to return a negative value to indicate an error and
2) The documentation for the traversal slot says: "If visit returns a non-zero value then an error has occurred and that value should be returned immediately." That is, the documentation says that the return value is an error indicator. I missed the bit about a non-zero return value and needing to return it. My bad.
Last night, Tim explained to me that a non-zero return value is not an error indicator. In fact, the GC system doesn't really allow errors. Rather, a non-zero return value provides a way for a visit proc to short-circuit the traversal process. As far as Tim knows, this feature has never been used. All visit functions in the core always return 0.
Alas, all my extensions that implement tp_traverse and the documentation I wrote on writing types is wrong and has to be changed. Sigh.
Should we call WHUI (we haven't used it!) on this feature that has been around for 4 versions of Python, that complicates tp_traverse implementations and hasn't been used? It would be simpler if we said that the return value could be ignored.
Jim
-- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
[Jim]
The documentation is still off I think, as it says that a non-zero value indicates an error, but I don't think that this is true.
I checked in doc chages, to clarify this, and to clarify that a NULL cannot be passed as the first arg to the visit() callback, and to clarify that users aren't normally expected to write visit() callbacks themselves (they're supplied by the core).
[Jim Fulton, on non-zero return values from a visitproc]
... Should we call WHUI (we haven't used it!) on this feature that has been around for 4 versions of Python, that complicates tp_traverse implementations and hasn't been used? It would be simpler if we said that the return value could be ignored.
As you and I (but not python-dev) discussed in later email, the non-zero return gimmick is used by two visitprocs in gcmodule.c after all. To match actual uses, instead of If visit returns a non-zero value then an error has occurred and that value should be returned immediately the docs should say If visit returns a non-zero value that value should be returned immediately I believe the latter matches the original design intent too. I doubt that anyone outside the core has written a visitproc. If so, all existing uses could just as well be met by If visit returns a non-zero value then 1 should be returned immediately but that wouldn't really save any cycles. Note that it's not expected that users will write visitproc. The type of visitproc has to be defined so that users can write tp_traverse functions, not really so they can write their own visitprocs. Any visitproc spec "that works" for gcmodule's internal purposes (gcmodule is the only known supplier of visitprocs) is good enough. User-supplied tp_traverse methods just have to play along. Moving the first last:
First, I don't like this macro, based on my own experience writing macros that hide returns. :)
Except VISIT is useful only inside a tp_traverse implementation, and all such implementations look exactly the same (VISIT, VISIT, VISIT, ..., sometimes with a surrounding loop). That's not an accident, since the only purpose of a tp_traverse implementation is to VISIT containees. So it's a special-purpose macro for a highly specific and uniform task, not a general-purpose macro that has to play well in arbitrary contexts.
Tim Peters wrote:
[Jim Fulton, on non-zero return values from a visitproc]
...
First, I don't like this macro, based on my own experience writing macros that hide returns. :)
Except VISIT is useful only inside a tp_traverse implementation, and all such implementations look exactly the same (VISIT, VISIT, VISIT, ..., sometimes with a surrounding loop). That's not an accident, since the only purpose of a tp_traverse implementation is to VISIT containees. So it's a special-purpose macro for a highly specific and uniform task, not a general-purpose macro that has to play well in arbitrary contexts.
I still don't like it, because it hides the return. I'd rather do: if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret; than VISIT(self->foo) So I'm not interested in the macro myself. :) I'll say again that I don't like calling the return from the visit proc an error indicator, because it's not, and because it goes against the common idiom of using a negative return to indicate an error. I think that these common idioms are important, because they help us understand the code we're reading more easily. Jim -- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
I still don't like it, because it hides the return.
It should be used in the core, but you don't have to use it in your own code.
I'd rather do:
if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret;
This violates the Python C style by putting an assignment in a conditional. You'd need to put the assignment to vret inside the body of an if statement. In other words, it would look just like the expansion of the VISIT() macro. Jeremy
Jeremy Hylton wrote:
I still don't like it, because it hides the return.
It should be used in the core, but you don't have to use it in your own code.
I'd rather do:
if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret;
This violates the Python C style by putting an assignment in a conditional.
Where is this documented? Assignments in conditionals are used widely in the C sources, including such places as gcmodule.c, object.c, and typeobject.c. Jim -- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
[Jeremy]
This violates the Python C style by putting an assignment in a conditional.
[Jim]
Where is this documented? Assignments in conditionals are used widely in the C sources, including such places as gcmodule.c, object.c, and typeobject.c.
It's not actually part of PEP 7 (the Python C style guidelines). It was an informal rule in PythonLabs (at least the non-Guido part of it <wink>), triggered by noting how many subtle bugs in real life ended up involving an embedded-in-a-conditional assignment. I don't run around getting rid of those for their own sake, but if I'm rewriting a piece of code for other reasons it's a transformation I routinely make now. And you can count the number of Python bugs traced to my C code over the last decade on one finger <wink -- but avoiding doing multiple things on a single line really helps, and so does avoiding side effects in the interior of expressions>.
[Jim Fulton]
I still don't like it, because it hides the return.
I'd rather do:
if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret;
than
VISIT(self->foo)
I don't believe you <0.5 wink>. Here, rewrite this: VISIT(self->readline); VISIT(self->read); VISIT(self->file); VISIT(self->memo); VISIT(self->stack); VISIT(self->pers_func); VISIT(self->arg); VISIT(self->last_string); VISIT(self->find_class); return 0; That's the current Unpickler_traverse, and it's utterly typical. In context, hiding the return doesn't matter: no computation is done between VISIT invocations.
So I'm not interested in the macro myself. :)
That's fair, but the lack of one will make correct docs for tp_traverse functions clumsier to write and read (you'll have to write out the tedious & error-prone (*because* tedious) expansion instead -- and that will encourage readers to code their tp_traverses in this brittle way too).
I'll say again that I don't like calling the return from the visit proc an error indicator, because it's not,
I agree (again <wink>).
and because it goes against the common idiom of using a negative return to indicate an error.
Well, it's not an error indicator (although it *may* be), so error idioms don't apply. I'll change that bit of the docs.
Tim Peters wrote:
[Jim Fulton]
I still don't like it, because it hides the return.
I'd rather do:
if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret;
than
VISIT(self->foo)
I don't believe you <0.5 wink>. Here, rewrite this:
VISIT(self->readline); VISIT(self->read); VISIT(self->file); VISIT(self->memo); VISIT(self->stack); VISIT(self->pers_func); VISIT(self->arg); VISIT(self->last_string); VISIT(self->find_class); return 0;
How about if ( 0 || || VISIT(self->readline) || VISIT(self->read) || VISIT(self->file) ) return 1; -- chris -- Christian Tismer :^) <mailto:tismer@stackless.com> Mission Impossible 5oftware : Have a break! Take a ride on Python's Johannes-Niemeyer-Weg 9a : *Starship* http://starship.python.net/ 14109 Berlin : PGP key -> http://wwwkeys.pgp.net/ work +49 30 89 09 53 34 home +49 30 802 86 56 mobile +49 173 24 18 776 PGP 0x57F3BF04 9064 F4E1 D754 C2FF 1619 305B C09C 5A3B 57F3 BF04 whom do you want to sponsor today? http://www.stackless.com/
[Christian Tismer]
How about
if ( 0 || || VISIT(self->readline) || VISIT(self->read) || VISIT(self->file) ) return 1;
Well, "0 ||" doesn't do anything at all, it doesn't return the return value of the visit() callback if a non-zero one is returned, it's clumsier to write and read than what Jim checked in, and since Py_VISIT does exactly one thing in exactly one context (tp_traverse implementations), I have no fear at all of the "hidden" return. Almost all tp_traverse implementations can (and should) look exactly alike: Py_VISIT(member1); Py_VISIT(member2); ... Py_VISIT(member_n); return 0; and Py_VISIT has no other use case. Why complicate it? Sticking a "return" in a general purpose macro could be horrid indeed; Py_VISIT is so specific, and of such limited use, there's simply nothing to fear here. It makes writing "correct by casual inspection" tp_traverse implementations dead easy, and has no effect on anything else. A continued +1 for the simplest thing that could possibly work here (which is happily a 100% solution in this case too).
Almost all tp_traverse implementations can (and should) look exactly alike:
Py_VISIT(member1); Py_VISIT(member2); ... Py_VISIT(member_n); return 0;
It does get a little more complicated for containers that loop over their contents and possibly have other PyObject * fields also. Still, I applied the macro to collections and itertools and found it to be straight-forward and easy to use. Raymond
if ( 0 || || VISIT(self->readline) || VISIT(self->read) || VISIT(self->file) ) return 1;
Yuck. Feels like double negatives, wheras the original felt "juuuuust right" :-) --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
if ( 0 || || VISIT(self->readline) || VISIT(self->read) || VISIT(self->file) ) return 1;
Yuck. Feels like double negatives, wheras the original felt "juuuuust right" :-)
I should have left out the "0 ||" which is nothing but a way to format all relevant lines the same. What I meant was just the or-ing together of the VISITs. -- Christian Tismer :^) <mailto:tismer@stackless.com> Mission Impossible 5oftware : Have a break! Take a ride on Python's Johannes-Niemeyer-Weg 9a : *Starship* http://starship.python.net/ 14109 Berlin : PGP key -> http://wwwkeys.pgp.net/ work +49 30 89 09 53 34 home +49 30 802 86 56 mobile +49 173 24 18 776 PGP 0x57F3BF04 9064 F4E1 D754 C2FF 1619 305B C09C 5A3B 57F3 BF04 whom do you want to sponsor today? http://www.stackless.com/
Jim Fulton wrote: [visit macro that returns]
I still don't like it, because it hides the return.
I'd rather do:
if (self->foo != NULL && (vret = visit(self->foo, arg))) return vret;
than
VISIT(self->foo)
Just as a side note on semantically strange macros: I did such a macro for easy error handling in Stackless months ago, which sets an error and returns. Before, the macro just had set an error. The changed behavior was edited correctly in all but two places, but I found the hidden error just by chance and very late. Such side effects of changed macros can normally be avoided by carefully avoiding type casts, to make sure that the compiler recognizes incompatible changes. Unfortunately, an embedded return statement bears *nothing* but changed semantics, and this cannot be detected by a compiler. This way, I managed to shoot myself seriously into the foot, and that although I was working alone on the project for weeks. I believe avoiding this is really a good idea. ciao - chris -- Christian Tismer :^) <mailto:tismer@stackless.com> Mission Impossible 5oftware : Have a break! Take a ride on Python's Johannes-Niemeyer-Weg 9a : *Starship* http://starship.python.net/ 14109 Berlin : PGP key -> http://wwwkeys.pgp.net/ work +49 30 89 09 53 34 home +49 30 802 86 56 mobile +49 173 24 18 776 PGP 0x57F3BF04 9064 F4E1 D754 C2FF 1619 305B C09C 5A3B 57F3 BF04 whom do you want to sponsor today? http://www.stackless.com/
Tim Peters <tim.peters@gmail.com> writes:
[Jim Fulton] ...
If there are no objections I'll add the following definition to object.h, after the definition for Py_DECREF:
#define Py_CLEAR(op) \ do { \ if (op) { \ PyObject *tmp = (op); \ (op) = NULL; \ Py_DECREF(tmp); \ } \ } while (0)
and update the docs and the tutorial on creating types in C.
+1. At least pystate.c can get rid of its functionally identical private ZAP macro then.
FWIW our experience in the C++ community suggests that deallocate-and-NULL tends to hide bugs. In general we prefer to use deallocate-and-fill-pointer-with-garbage. I'm not sure if the experience translates here, of course. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
[David Abrahams, on the Py_CLEAR macro]
FWIW our experience in the C++ community suggests that deallocate-and-NULL tends to hide bugs. In general we prefer to use deallocate-and-fill-pointer-with-garbage. I'm not sure if the experience translates here, of course.
I don't think so. Along with the macro, Jim checked in a nice explanation in the Extending and Embedding manual. A decref can cause anything to happen if the decref'ed containee is now trash, including fresh accesses to the container object holding the now-trash containee that was decrefed. If the container's pointer to the containee isn't NULLed out first, the entire implementation of the container has to be aware that this particular containee may hold an insane value. Ditto if garbage is put in the pointer. Ditto for NULL too, but NULL is a single, obvious, and easy-to-test way to say "and this containee no longer exists". This isn't really so much about deallocation as about horrid side effects triggered by deallocation.
[David Abrahams, on the Py_CLEAR macro]
FWIW our experience in the C++ community suggests that deallocate-and-NULL tends to hide bugs. In general we prefer to use deallocate-and-fill-pointer-with-garbage. I'm not sure if the experience translates here, of course.
[Tim]
I don't think so. Along with the macro, Jim checked in a nice explanation in the Extending and Embedding manual. A decref can cause anything to happen if the decref'ed containee is now trash, including fresh accesses to the container object holding the now-trash containee that was decrefed. If the container's pointer to the containee isn't NULLed out first, the entire implementation of the container has to be aware that this particular containee may hold an insane value. Ditto if garbage is put in the pointer. Ditto for NULL too, but NULL is a single, obvious, and easy-to-test way to say "and this containee no longer exists". This isn't really so much about deallocation as about horrid side effects triggered by deallocation.
Plus, in many cases NULL is *already* an expected value. We even have macros like Py_XDECREF() that are NULL-aware. --Guido van Rossum (home page: http://www.python.org/~guido/) --i6G2WKR31130.1089945140/guido.python.org--
Guido van Rossum <guido@python.org> writes:
[David Abrahams, on the Py_CLEAR macro]
FWIW our experience in the C++ community suggests that deallocate-and-NULL tends to hide bugs. In general we prefer to use deallocate-and-fill-pointer-with-garbage. I'm not sure if the experience translates here, of course.
[Tim]
I don't think so. Along with the macro, Jim checked in a nice explanation in the Extending and Embedding manual. A decref can cause anything to happen if the decref'ed containee is now trash, including fresh accesses to the container object holding the now-trash containee that was decrefed. If the container's pointer to the containee isn't NULLed out first, the entire implementation of the container has to be aware that this particular containee may hold an insane value. Ditto if garbage is put in the pointer. Ditto for NULL too, but NULL is a single, obvious, and easy-to-test way to say "and this containee no longer exists". This isn't really so much about deallocation as about horrid side effects triggered by deallocation.
Plus, in many cases NULL is *already* an expected value.
We even have macros like Py_XDECREF() that are NULL-aware.
That's exactly the sort of case that covers up bugs in C++. Our operator delete basically just ignores NULL values. When people write delete p; p = 0; It can cover up bugs where p is unintentionally getting deleted twice. It might still not be applicable here, though. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
[David Abrahams]
That's exactly the sort of case that covers up bugs in C++.
Our operator delete basically just ignores NULL values. When people write
delete p; p = 0;
It can cover up bugs where p is unintentionally getting deleted twice. It might still not be applicable here, though.
Understood. Py_CLEAR isn't about deallocation, it's about a container releasing *a* reference it owns to a containee. The container doesn't know, and doesn't care, whether it's releasing the last reference to the containee. Once a container no longer refers to a containee, that's all the NULL means -- "I no longer refer to anything here", not necessarily "the thing I referenced is no longer referenced by anyone and has been recycled". The former containee *may* get recycled at this point, and it's important for the container to record that it no longer references it before any destructors associated with the containee get executed. This is harmless. But it's something people often do in a wrong order, which is why it's a Good Idea to have a macro capturing the right order. A wrong order can lead to horridly subtle bugs.
Jim> The correct way to do this is something like: Jim> tmp = self->foo; Jim> self-> foo = NULL; Jim> Py_XDECREF(tmp); Jim> I suggest that there should be a standard macro to automate this. Jim> Py_CLEAR(self->foo) Jim> This would be defined to be the same result as Py_XDECREF except Jim> that the argument will be set to NULL. Jim> Thoughts? Just my two cents, but the name suggests to me that it's doing more than a simple decref and set to NULL. I can't think of anything else off the top of my head that seems obviously better though. Skip
Skip Montanaro <skip@pobox.com>:
Just my two cents, but the name suggests to me that it's doing more than a simple decref and set to NULL. I can't think of anything else off the top of my head that seems obviously better though.
Py_XDECREF_NULL? Greg Ewing, Computer Science Dept, +--------------------------------------+ University of Canterbury, | A citizen of NewZealandCorp, a | Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. | greg@cosc.canterbury.ac.nz +--------------------------------------+
Just my two cents, but the name suggests to me that it's doing more than a simple decref and set to NULL. I can't think of anything else off the top of my head that seems obviously better though.
Py_XDECREF_NULL?
Bleah. In pystate.c you'll find: #define ZAP(x) { \ PyObject *tmp = (PyObject *)(x); \ (x) = NULL; \ Py_XDECREF(tmp); \ } Call it Py_ZAP and you have my vote. :-) --Guido van Rossum (home page: http://www.python.org/~guido/)
participants (9)
-
Christian Tismer -
David Abrahams -
Greg Ewing -
Guido van Rossum -
Jeremy Hylton -
Jim Fulton -
Raymond Hettinger -
Skip Montanaro -
Tim Peters