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

Fixing wrapper_heap alignment problems #3007

Merged
merged 2 commits into from Dec 2, 2017
Merged

Fixing wrapper_heap alignment problems #3007

merged 2 commits into from Dec 2, 2017

Conversation

sithhell
Copy link
Member

This patch ensures that the addresses returned from the wrapper_heap are
correctly aligned for the underlying objects.

This patch ensures that the addresses returned from the wrapper_heap are
correctly aligned for the underlying objects.
Conflicts:
	hpx/runtime/components/server/wrapper_heap_list.hpp
@sithhell
Copy link
Member Author

This PR should be ready now.

Copy link
Contributor

@biddisco biddisco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me, though I only 'reviewd' the minor changes to use a heap params struct.

first_free_ = pool_;
size_ = s / element_size_; //-V104
free_size_ = size_;
first_free_ = (reinterpret_cast<std::size_t>(pool_)
Copy link
Contributor

@biddisco biddisco Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uintptr_t maybe for the first_free usage throughout?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::size_t is good enough as it is guaranteed to be able to hold pointers.

@sithhell
Copy link
Member Author

sithhell commented Dec 1, 2017

@hkaiser do you still want to test this on windows? Appveyor is fine with it. I'd really like to move on here, the bug that's fixed here is really severe.

@hkaiser
Copy link
Member

hkaiser commented Dec 1, 2017

@sithhell I'd appreciate it if I had a bit more time to test this on windows.

@sithhell
Copy link
Member Author

sithhell commented Dec 2, 2017

[22:15:21] <hkaiser> heller: pls go ahead with merging #3007, I won't have the time to thoroughly investigate things right now
[22:15:39] <hkaiser> if it breaks something we'll fix it later

@sithhell sithhell merged commit 0ca24f8 into master Dec 2, 2017
@sithhell sithhell deleted the fix_wrapper_heap branch December 2, 2017 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants