BUG: index overlap copy #26958
BUG: index overlap copy #26958
Conversation
|
I'm not sure whether I understand the source correctly, but shouldn't we use |
|
Sebastian asked me to use But I don't really have strong views on it, I just tend to do what Sebastian suggests :) |
|
I'd follow Seb's suggestion. Just copy.deepcopy module seems to be a little overkill for me. |
That function isn't used in this file at all (or its no-copy version). It does some additional setup, and is used by
Of course I prefer if you don't ;), but I guess it works. FWIW, it may make a lot of sense to move these checks into
True, we know that the inputs are arrays, so we can copy them as arrays. |
60b62ca to
dcfe5d8
Compare
|
@seberg I revised to do an array copy instead of the I didn't do the extra step of trying to migrate to |
|
@seberg do you want me to make the extra changes here or is this "ok?" |
numpy/_core/src/multiarray/mapping.c
Outdated
| @@ -1963,6 +1963,9 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op) | ||
| if (tmp_arr && solve_may_share_memory(self, tmp_arr, 1) != 0) { | ||
| Py_SETREF(tmp_arr, (PyArrayObject *)PyArray_NewCopy(tmp_arr, NPY_ANYORDER)); | ||
| } | ||
| if (index_has_memory_overlap(self, index_type, indices, index_num, (PyObject *)tmp_arr) == 1) { | ||
| Py_SETREF(indices->object, PyArray_NewCopy((PyArrayObject *)indices->object, NPY_KEEPORDER)); |
There was a problem hiding this comment.
Sorry, I don't know how I missed both pings. This doesn't work, indices-> is a C array of index information.
If you add a test like:
a[2:][None, a[:-2]] = 3
that should just segfault. The other place where the index_has_memoverlap is used actually copies the result (self here and then uses the Writebackifcopy mechanism to make that work).
Now, it may be however, that this is sufficient, because it could be that this only is relevant to the fast path (because otherwise the iterator setup can protect us).
Either way, not sure that you want to look at it again, which is fine... Just realized that I never responded.
* Fixes numpy#26844 * Perform a defensive copy of array indices when the indices themselves may have memory overlap with `self`.
* Revise the fix for index memory overlap to use a regular array copy on the indices rather than a deepcopy, which was a bit more heavyweight than needed.
* Add regression test for segfault from: https://github.com/numpy/numpy/pull/26958/files#r1854589178 * Add a fix for the above bug after discussing in person with Sebastian.
dcfe5d8 to
f70e1e1
Compare
|
I added an extra regression test and bug fix for the scenario described at #26958 (comment), after discussing this PR with @seberg in person at the summit today. |
|
Thanks, Tyler! Let's give this a shot. I could imagine that it could be better to copy |
Fixes BUG: memory overlap in some tricky indexing scenarios #26844
Perform a defensive copy of array indices when the indices themselves may have memory overlap with
self.It does feel a bit complicated, though the tests passed locally. I was hoping to avoid usage of both
solve_may_share_memoryandindex_has_memory_overlapin succession, since the latter contains the former, but then I think I'd need to perform both defensive copies. I wonder if it might be useful forindex_has_memory_overlapto have a different return code for index vs. "other" overlap so that the defensive copy might be performed selectively to the type of overlap. I'm guessing my ref counting is probably missing some stuff too.