Give PyArray<PyObject> another try.

[adamreichold]

I am not sure what changed since #143 (comment) or whether the segmentation fault was only triggered by a more involved test but this seems to work using Python 3.8.2 on Linux. (Similarly to how #138 (comment) worked in this environment.)

Fixes #175

[adamreichold]

the segmentation fault was only triggered by a more involved test but this seems to work using Python 3.8.2 on Linux

Ah, it is not deterministic! Executed often enough, it is triggered...

[adamreichold]

And it also seems to trigger only if the other tests in the that binary are executed.

[kngwyu]

Yeah, I'm sorry but I also don't have much intuition about this bug. I just confirmed SIGSEGV is triggered in PyFinalizeEx using gdb.

[adamreichold]

It seems to be a double-free, i.e. if I add mem::forget(vec) to the test, the problems seems to go away. Could it be that .to_pyarray() should either take ownership or clone (and there INCREF) the items?

[adamreichold]

Yeah, ToPyArray goes via PyArray::from_slice which just does array.copy_ptr(slice.as_ptr(), slice.len());, i.e. copying the pointer values in case of objects but not increasing their element counts. I think the above should only happen for Copy types.

[adamreichold]

Oh, I think PyArray::from_slice is generally unsound as it copies any type T whether that is cloneable or not.

[adamreichold]

I pushed a fix for PyArray::from_slice even though it is probably slower for Copy types. Will look into specialisation next.

@kngwyu One thing unrelated I wonder is why PyArray::new is not unsafe? It does create an uninitialized array which would then allow me to access uninitialized memory, wouldn't it?

[adamreichold]

I pushed a fix for PyArray::from_slice even though it is probably slower for Copy types. Will look into specialisation next.

I found one more place where A: Element was assumed to imply A: Copy and copy_ptr was used in ToPyArray for ArrayBase.

However, I do not think ensuring a call to copy_nonoverlapping is possible without actual specialisation support on stable. The write-clone-into-offset-pointer idiom used now should give LLVM the opportunity to make that optimization though.

I also wrapped those partially initialized arrays into ManuallyDrop to avoid freeing uninitialized pointers but I think longer term, we'd want to support PyArray<MaybeUninit<T>> similar to ndarray.

[kngwyu]

Great

It seems to be a double-free, i.e. if I add mem::forget(vec) to the test, the problems seems to go away.

Ah, that's an awesome finding, thanks.

One thing unrelated I wonder is why PyArray::new is not unsafe? It does create an uninitialized array which would then allow me to access uninitialized memory, wouldn't it?

Yeah, your understanding is correct and it should be unsafe.

However, I do not think ensuring a call to copy_nonoverlapping is possible without actual specialisation support on stable. The write-clone-into-offset-pointer idiom used now should give LLVM the opportunity to make that optimization though.

I do think that throwing out copy_nonoverlapping is OK for now.

[adamreichold]

Yeah, your understanding is correct and it should be unsafe.

Opened #217 to discuss how to solve this as it seems unrelated to this change and should probably result in new API.

[adamreichold]

I do think that throwing out copy_nonoverlapping is OK for now.

I added another commit which takes care of properly leaking uninitialized PyObject arrays to avoid heap corruption which has the nice side effect of restoring that optimization since the other NumPy types are assumed to be Copy by NumPy itself anyway.

[davidhewitt]

To avoid leaking memory if clone panics, you could use a guard pattern similar to https://github.com/PyO3/pyo3/blob/00c84eb0baec6f41623e83737a291d3e0d30cc5b/src/conversions/array.rs#L93

You would need to drop the initialized elements by hand and then zero the array so that numpy can deallocate safely I guess.

[adamreichold]

Certainly, however I prefer to postpone this to separate PR after we have reached soundness w.r.t PyArray creation.

[davidhewitt]

👍 makes sense, let's just remember to open an issue for this when this PR merges.

[adamreichold]

I took a different approach for now:

// Use zero-initialized pointers for object arrays
// so that partially initialized arrays can be dropped safely
// in case the iterator implementation panics.
let array = if T::DATA_TYPE == DataType::Object {
    Self::zeros(py, [iter.len()], false)
} else {
    Self::new(py, [iter.len()], false)
};

This way we start out with null pointers in the object case and the array is always safe to drop. I think the overhead is warranted due to the cost that arrays of pointers imply in any case. (Of course, using a guard that just zeros out uninitialized array elements if there actually is a panic can still be implemented as a follow-up optimization.)

[davidhewitt]

Oh very nice!

[kngwyu]

Yeah, it looks a good temporal solution 👍🏼

[adamreichold]

Sorry for the notification spam, but I wanted to add that I rebooted this patch series again to make the contract of trait Element more explicit and then do the minimal changes to make PyArray<PyObject> sound. This yields a much smaller diff without any intrusive changes affecting the existing code.

[davidhewitt]

Looks great to me, nice work! Could perhaps add extra tests for from_slice for PyObject vec etc

[adamreichold]

Could perhaps add extra tests for from_slice for PyObject vec etc

I suppose you mean extra test for impl<S, D, A> ToPyArray for ArrayBase<S, D> as PyArray::from_slice is what is exercised by the current test (via impl<T> ToPyArray for [T])?

Will add a test case for starting from an ndarray array...

[kngwyu]

Thanks!
This is a huge step.

@adamreichold
Just let me confirm: we can merge this PR as is and you want to refactor Element in a separate PR, right?

[adamreichold]

we can merge this PR as is and you want to refactor Element in a separate PR, right?

Yes, this PR is good to go in my opinion. I don't think we will need to touch trait Element, but I would like to work on a follow-up to give PyArray::new a different signature and documentation, i.e. work on #217

[kngwyu]

👍🏼