[Python-Dev] Preserving the blamelist (original) (raw)

Tim Peters tim.peters at gmail.com
Wed Apr 12 19:43:43 CEST 2006

[Tim]

Phillip, when eyeballing gendealloc(), I didn't understand two things:

1. Why doesn't if (gen->giframe->fstacktop!=NULL) { check first to be sure that gen->giframe != PyNone?

[Phillip]

Apparently, it's because I'm an idiot, and because nobody else realized this during the initial review of the patch. :)

Then you were the bold adventurer, and the reviewers were the idiots ;-)

Is that impossible here for some reason?

No, I goofed. It's amazing that this doesn't dump core whenever a generator exits. :(

Well, it's extremely likely that &((PyGenObject *)Py_None)->f_stacktop is a legit part of the process address space, so that dereferencing is non-problematic. We just read up nonsense then, test against it, and the conditional

    gen->ob_type->tp_del(self);

is harmless as gen_del() returns at once (because it does check for Py_None right away -- we never try to treat gi_frame->f_stacktop as a frame pointer here, we just check it against NULL). If we have to have insane code, harmlessly insane is the best kind :--)

2. It looks like "giframe != NULL" is an (undocumented) invariant. Right? If so,

PyXDECREF(gen->giframe); sends a confusing message (because of the "X", implying that NULL is OK). Regardless, it would be good to add comments to genobject.h explaining the possible values giframe can hold. For example, what does it mean when giframe is PyNone? Can it ever be NULL?

I think what happened is that at one point I thought I was going to set giframe=NULL when there's no active frame, in order to speed up reclamation of the frame. However, I think I then thought that it would break the operation of the generator's 'giframe' attribute, which is defined as TOBJECT.

That shouldn't be a problem: when PyMember_GetOne() fetches a T_OBJECT that's NULL, it returns (a properly incref'ed) Py_None instead.

Or else I thought that the tpvisit was screwed up in that case.

So, it looks to me like what this should do is simply allow giframe to be NULL instead of PyNone, which would get rid of all the silly casting, and retroactively make the XDECREF correct. :)

That indeed sounds better to me, although you still don't get out of writing gi_frame comments for genobject.h :-)

Does gentraverse() need to do anything special to visit a null pointer? Should it only conditionally visit it?

The body of gen_traverse() is best written:

Py_VISIT(gen->gi_frame);
return 0;

Py_VISIT is defined in objimpl.h, and takes care of NULLs and exceptional returns.

BTW, someone looking for an easy task might enjoy rewriting other tp_traverse slots to use Py_VISIT. We even have cases now (like super_traverse) where modules define their own workalike traverse-visit macros, which has become confusing since a standard macro was defined for this purpose.

More information about the Python-Dev mailing list