Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unaligned memory accesses in conversion functions #904

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

jrtc27
Copy link
Contributor

@jrtc27 jrtc27 commented Jul 10, 2017

No guarantees are provided for the alignment of the buffers used for the various conversion functions, and in the case of packed structs (easily creatable with np.dtype, and in fact the test suite does so on the odd occasion) they definitely can be unaligned. Therefore exclusively use memcpy for multi-byte accesses to these buffers. The test suite now passes on Linux sparc64, which was previously crashing due to unaligned accesses.

No guarantees are provided for the alignment of the buffers used for the
various conversion functions, and in the case of packed structs (easily
creatable with np.dtype, and in fact the test suite does so on the odd
occasion) they definitely can be unaligned. Therefore exclusively use
memcpy for multi-byte accesses to these buffers. The test suite now
passes on Linux sparc64, which was previously crashing due to unaligned
accesses.
@jrtc27
Copy link
Contributor Author

jrtc27 commented Jul 10, 2017

AppVeyor failure is due to the Python35 (32-bit) build:

Failed to download hdf5 version 1.8.17, exiting

@aragilar
Copy link
Member

Reran appveyor, LGTM

@tacaswell
Copy link
Member

Thanks!

I'll let this set a bit longer to see if it attracts any other attention, but 👍 to merging.

Between this and #903 I think we should to a 2.7.1 next week.

@aragilar
Copy link
Member

@tacaswell That sounds good

@ghisvail
Copy link
Contributor

@tacaswell Feel free to ping me when 2.7.1 is out so I can update the Debian packaging. Thanks.

@tacaswell
Copy link
Member

@ghisvail If I recall correctly we were seeing failures on sparc in the debian build system. Do I recall correctly and would it be possible to get this run through that before merging this? Is the cost of getting that tested an rc ;) ?

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jul 13, 2017

@tacaswell I actually tested this patch by adding it to the (admittedly, 2.7.0) Debian package and building on a sparc64 machine

@ghisvail
Copy link
Contributor

I fully trust James on this. I don't think an RC is necessary, better just to roll a point release out and iterate if need be.

@tacaswell tacaswell merged commit 461e2f4 into h5py:master Jul 18, 2017
@ghisvail
Copy link
Contributor

@tacaswell Are you planning a new release, or shall I apply the patch to the packaging?

@tacaswell
Copy link
Member

Hopefully this week.....I am thoroughly over-subscribed

@jrtc27
Copy link
Contributor Author

jrtc27 commented Aug 15, 2017

Friendly ping :)

@jrtc27 jrtc27 deleted the unaligned-access branch August 15, 2017 06:52
@tacaswell
Copy link
Member

Making progress, see #913

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants