On Sun, Oct 18, 2009 at 6:04 AM, Michael Droettboom <md...@stsci.edu> wrote:
> On 10/16/2009 11:35 PM, Travis Oliphant wrote:
> >
> > On Oct 15, 2009, at 11:40 AM, Michael Droettboom wrote:
> >
> >> I recently committed a regression test and bugfix for object pointers in
> >> record arrays of unaligned size (meaning where each record is not a
> >> multiple of sizeof(PyObject **)).
> >>
> >> For example:
> >>
> >> a1 = np.zeros((10,), dtype=[('o', 'O'), ('c', 'c')])
> >> a2 = np.zeros((10,), 'S10')
> >> # This copying would segfault
> >> a1['o'] = a2
> >>
> >> http://projects.scipy.org/numpy/ticket/1198
> >>
> >> Unfortunately, this unit test has opened up a whole hornet's nest of
> >> alignment issues on Solaris. The various reference counting functions
> >> (PyArray_INCREF etc.) in refcnt.c all fail on unaligned object pointers,
> >> for instance. Interestingly, there are comments in there saying
> >> "handles misaligned data" (eg. line 190), but in fact it doesn't, and
> >> doesn't look to me like it would. But I won't rule out a mistake in
> >> building it on my part.
> >
> > Thanks for this bug report. It would be very helpful if you could
> > provide the line number where the code is giving a bus error and
> > explain why you think the code in question does not handle misaligned
> > data (it still seems like it should to me --- but perhaps I must be
> > missing something --- I don't have a Solaris box to test on).
> > Perhaps, the real problem is elsewhere (such as other places where the
> > mistake of forgetting about striding needing to be aligned also before
> > pursuing the fast alignment path that you pointed out in another place
> > of code).
> >
> > This was the thinking for why the code (that I think is in question)
> > should handle mis-aligned data:
> >
> > 1) pointers that are not aligned to the correct size need to be copied
> > to an aligned memory area before being de-referenced.
> > 2) static variables defined in a function will be aligned by the C
> > compiler.
> >
> > So, what the code in refcnt.c does is to copy the value in the NumPy
> > data-area (i.e. pointed to by it->dataptr) to another memory location
> > (the stack variable temp), dereference it and then increment it's
> > reference count.
> >
> > 196: temp = (PyObject **)it->dataptr;
> > 197: Py_XINCREF(*temp);
> This is exactly an instance that fails. Let's say we have a PyObject at
> an aligned location 0x4000 (PyObjects themselves always seem to be
> aligned -- I strongly suspect CPython is enforcing that). Then, we can
> create a recarray such that some of the PyObject*'s in it are at
> unaligned locations. For example, if the dtype is 'O,c', you have a
> record stride of 5 which creates unaligned PyObject*'s:
>
> OOOOcOOOOcOOOOc
> 0123456789abcde
> ^ ^
>
> Now in the code above, let's assume that it->dataptr points to an
> unaligned location, 0x8005. Assigning it to temp puts the same
> unaligned value in temp, 0x8005. That is:
>
> &temp == 0x1000 /* The location of temp *is* on the stack and aligned */
> temp == 0x8005 /* But its value as a pointer points to an unaligned
> memory location */
> *temp == 0x4000 /* Dereferencing it should get us back to the original
> PyObject * pointer, but dereferencing an
> unaligned memory location
> fails with a bus error on Solaris */
>
> So the bus error occurs on line 197.
>
> Note that something like:
>
> PyObject* temp;
> temp = *(PyObject **)it->dataptr;
>
> would also fail.
>
> The solution (this is what works for me, though there may be a better way):
>
> PyObject *temp; /* NB: temp is now a (PyObject *), not a (PyObject
> **) */
> /* memcpy works byte-by-byte, so can handle an unaligned assignment */
> memcpy(&temp, it->dataptr, sizeof(PyObject *));
> Py_XINCREF(temp);
>
> I'm proposing adding a macro which on Intel/AMD would be defined as:
>
> #define COPY_PYOBJECT_PTR(dst, src) (*(dst) = *(src))
>
> and on alignment-required platforms as:
>
> #define COPY_PYOBJECT_PTR(dst, src) (memcpy((dst), (src),
> sizeof(PyObject *))
>
> and it would be used something like:
>
> COPY_PYOBJECT_PTR(&temp, it->dataptr);
>
>
This looks right to me, but I'll let Travis sign off on it.
<snip>
Chuck
_______________________________________________
NumPy-Discussion mailing list
NumPy-Discussion@scipy.org
http://mail.scipy.org/mailman/listinfo/numpy-discussion
