[Python-Dev] Add Py_REPLACE and Py_XREPLACE macros

[Nick Coghlan]

On 16 February 2014 02:52, Serhiy Storchaka <storchaka at gmail.com> wrote:
> There are objections to these patches. Raymond against backporting the patch
> unless some known bugs are being fixed [1]. But it fixes at least one bug
> that caused a crash. And I suspect that there may be other bugs, just we
> still have no reproducers. Even if we don't know how to reproduce the bug,
> the current code looks obviously wrong. Also porting the patch will make the
> sync easier. Note that the patches were automatically generated, which
> reduces the possibility of errors. I just slightly corrected formatting,
> remove unused variables and fixed one error.

It's also likely than many of these crashes could only be reproduced
through incorrect usage of the C API.

Py_CLEAR/Py_XCLEAR was relatively simple, as there's only one pointer involved.

As Martin noted, Py_REPLACE is more complex, as there's the question
of whether or not the refcount for the RHS gets incremented.

Rather than using a completely different name, perhaps we can leverage
"Py_CLEAR" to make it more obvious that this operation is "like
Py_CLEAR and for the same reason, just setting the pointer to
something other than NULL".

For example:

    Py_CLEAR_AND_SET
    Py_XCLEAR_AND_SET

Such that Py_CLEAR and Py_XCLEAR are equivalent to:

    Py_CLEAR_AND_SET(target, NULL);
    Py_XCLEAR_AND_SET(target, NULL);

(Note that existing occurrences of SET in C API macros like
PyList_SET_ITEM and PyTuple_SET_ITEM also assume that any required
reference count adjustments are handled externally).

While the name does suggest the macro will expand to:

    Py_CLEAR(target);
    target = value;

which isn't quite accurate, it's close enough that people should still
be able to understand what the operation does. Explicitly pointing out
in that docs that Py_CLEAR(target) is functionally equivalent to
Py_CLEAR_AND_SET(target, NULL) should help correct the
misapprehension.

On the question of updating 3.4+ only vs also updating 3.3, I'm
inclined towards fixing it systematically in all currently supported
branches, as it's a low risk mechanical change. On the other hand, we
still have other crash bugs with known reproducers (in
Lib/test/crashers), so I'm also OK with leaving 3.3 alone. (Assuming
we can agree on a name, I definitely think it's worth asking Larry for
permission to make the late C API addition for 3.4, though)

Regards,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia