Fix leak in var arg handling #1216
Conversation
This time caused by an upstream pybind11 bug: pybind/pybind11#1216 This changes causes the code to go down a non-buggy pathway.
|
Can you add a test case to the PR that triggers the warning (without the fix) and passes (with the fix). It looks from your description like you already have something along those lines. |
include/pybind11/pybind11.h
Outdated
| @@ -581,7 +581,7 @@ class cpp_function : public function { | |||
| extra_args = tuple(args_size); | |||
| for (size_t i = 0; i < args_size; ++i) { | |||
| handle item = PyTuple_GET_ITEM(args_in, args_copied + i); | |||
| extra_args[i] = item.inc_ref().ptr(); | |||
| extra_args[i] = reinterpret_borrow<object>(item); | |||
There was a problem hiding this comment.
The reinterpret_borrow and the temporary variable aren't actually needed; the two lines can be simplified to just:
extra_args[i] = PyTuple_GET_ITEM(args_in, args_copied + i);
|
Bug confirmed (this was introduced in the dispatcher overhaul in #611). See my suggested simplification, add a simple test case (so that it can't reoccur), and I'll merge. |
|
Actually, you can ignore my suggestions; I've written some test code, and I'll push it (and the minor code simplification) to this PR soon. |
* Fix another leak in pybind11 code. This time caused by an upstream pybind11 bug: pybind/pybind11#1216 This changes causes the code to go down a non-buggy pathway. * Relax verify of VariableFlags If we trace with a defined tensor, but see a run with a undefined tensors we now allow that run to happen, replacing the tensor with zeros. This also fixes a bug where stage 0 tensors were not checked against their verify flags. This change does _not_ handle all bad situations that can happen. For instance if the first thing traced has a undefined tensor but a later tensor is defined, then it will fail because the graph itself does not contain the trace for the derivative of the tensor. However it is possible to work around this later case by dry-running the function: z = Variable(...,requires_grad=True) x,y = f(z) (x.sum() + y.sum()).backward()
This time caused by an upstream pybind11 bug: pybind/pybind11#1216 This changes causes the code to go down a non-buggy pathway.
|
Sounds good, thanks for writing the test code! |
|
Pushed the simplification and test cases. (I also ended up rebasing your branch against current master to incorporate the MSVC fix I just pushed there). Assuming it all builds successfully I'll go ahead with the merge soon. |
When using the mixed position + vararg path, pybind over inc_ref's the vararg positions. Printing the ref_count() of `item` before and after this change you see: Before change: ``` refcount of item before assign 3 refcount of item after assign 5 ``` After change ``` refcount of item before assign 3 refcount of item after assign 4 ```
|
Pushed a PyPy test fix; hopefully that does it. |
|
Merged! Thanks for the PR. |
When using the mixed position + vararg path, pybind over inc_ref's
the vararg positions. Printing the ref_count() of
itembeforeand after this change you see:
Before change:
After change