MAINT: avoid memcpy when i == j #8921
Conversation
Valgrind complains about memcpy with overlapping address in mtrand.c It happens when i == j in this loop. Closer inspection the i == j iteration is not needed (it is a swap). So, skip it and avoid depending on undefined behavior of memcpy. related read: https://sourceware.org/bugzilla/show_bug.cgi?id=12518
| @@ -4828,6 +4828,7 @@ cdef class RandomState: | |||
| cdef npy_intp i, j | |||
| for i in reversed(range(1, n)): | |||
| j = rk_interval(i, self.internal_state) | |||
| if i == j : continue # i == j is not needed and memcpy is undefined. | |||
There was a problem hiding this comment.
just changing the rk_interval i to i -1 should be enough
There was a problem hiding this comment.
no that is wrong, disregard
There was a problem hiding this comment.
That would work, but would change the behaviour of a given random seed, right?
There was a problem hiding this comment.
worse it would mean a datapoint cannot stay in the same position after a shuffle which biases the result
|
Full overlap memcpy is actually fine. |
|
small memcpy is also 'actually' fine (see the bz discussion), but since the spec says overlapped areas is undefined I think avoiding overlapping memcpy is the safest/portable approach; also rk_internal and the loop looked sufficiently complicated that adding an |
|
I don't really like changing the generated code to silence a non-issue, too bad we can't use memmove as older gccs won't inline that. |
|
uhm dumb question to ask after pressing the button, but where exactly is the overlap in this code? memcpy is done into a temporary buffer here. |
|
nevermind I see it ... my valgrind version just does not care about full overlap anymore. |
Valgrind complains about memcpy with overlapping address in mtrand.c
It happens when i == j in this loop.
Closer inspection the i == j iteration is not needed (it is a swap).
So, skip it and avoid depending on undefined behavior of memcpy.
related read:
https://sourceware.org/bugzilla/show_bug.cgi?id=12518