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 problems with is_bitwise_serializable #2168

Closed
wants to merge 1 commit into from

Conversation

sithhell
Copy link
Member

The newly introduced switch to is_trivially_copyable introduced problems with
std::true_type/false_type vs boost::mpl::true_/false_. This patch is attempting
to fix those issues.

The newly introduced switch to is_trivially_copyable introduced problems with
std::true_type/false_type vs boost::mpl::true_/false_. This patch is attempting
to fix those issues.
@hkaiser
Copy link
Member

hkaiser commented May 19, 2016

LGTM, thanks!

@K-ballo
Copy link
Member

K-ballo commented May 19, 2016

What was the problem?

@hkaiser
Copy link
Member

hkaiser commented May 19, 2016

What was the problem?

It was a mixup between mpl::bool_ and std::constant_variable<bool, ...>

@K-ballo
Copy link
Member

K-ballo commented May 19, 2016

Could you be more precise? There were a number of changes in place to work only with X::value, so that the actual type of the trait result would not actually matter.

@sithhell
Copy link
Member Author

specifically it was hpx::util::tuple, hpx::parcelset::parcel::data and hpx::actions::detail::action_serialization_data.

@K-ballo
Copy link
Member

K-ballo commented May 19, 2016

That still doesn't tell me much. What was the problem? Why didn't the measures in place failed (the ones reverted in this PR), were there missing in certain places?
How do I reproduce the issue?

@sithhell
Copy link
Member Author

There was nothing reverted in this PR. you can reproduce it by running hello_world on two localities. I used clang 3.8.0 with ubsan and boost 1.60.0. However, I just discovered that the problem still seems to exist when compiling everything with asan.

@K-ballo
Copy link
Member

K-ballo commented May 19, 2016

This PR reverts a bunch of std::integral_constant<bool, X::value> to just X again, which were introduced with the bitwise-serializable changes so to avoid std::true_type/false_type vs boost::mpl::true_/false_ problems.

I would like to know why those changes failed to avoid the issue this PR is solving. Did I miss those fixes somewhere? Which files and lines make calls that will pick the wrong overload without this patch?

@sithhell
Copy link
Member Author

Actually, I am not convinced right now that this patch fixes the issue with the continuation serialization for good. However, the changes that you claim that got reverted are mainly because I changed all specializations of is_bitwise_serializable in the hpx codebase to return std::true_type/std::false_type so we don't need that rewrapping. As said in another comment, I encountered this problem again. However, I think this patch is valid anyway as it removed some inconsistencies wrt boost.MPL and std type_traits.

@K-ballo
Copy link
Member

K-ballo commented May 19, 2016

There should be no inconsistencies between Boost.MPL and std type_traits, the code is intended to handle both. If there are, it's a bug and it should be fixed.

That said, I think it's a good idea to keep the code simple by requiring std::true/false_type base characteristics. I originally started doing this migration, on the premise that if a user explicitly specialized the trait instead of using the macro it was their own fault. Then I hit cases like tuple, where the macro does not help users, and reverted those changes. I believe a proper fix would be to introduce another macro to work with templates, one that takes a bool constant and specializes the trait as needed.

typedef std::integral_constant<bool,
hpx::traits::is_bitwise_serializable<T>::value> use_optimized;
typedef typename
hpx::traits::is_bitwise_serializable<T>::type use_optimized;
Copy link
Member

Choose a reason for hiding this comment

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

This (and other similar changes) should just be hpx::traits::is_bitwise_serializable<T>, as the trait would be required to derive from std::integral_constant<bool, C> as other traits do.

@sithhell
Copy link
Member Author

The changes made in this PR turned out to be a red herring. The commit that causes problems is f854314. We need to investigate further why this change caused problems like these: https://gist.github.com/sithhell/c5a21ae5267c53783d3899182659783d

@sithhell sithhell closed this May 19, 2016
@sithhell sithhell deleted the fix_serialization branch May 19, 2016 19:50
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