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

#4913: Make View self-assignment not produce double-free #5024

Merged
merged 4 commits into from
May 17, 2022

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented May 16, 2022

Replacing #4914 because I can neither push to NexGenAnalytics organization nor, do a PR against that fork??

Fixes #4913

Comment on lines 1090 to 1093
// Test self assignment
dx = dx;
ASSERT_EQ(dx.use_count(), 1);
dx = reinterpret_cast<typename dView4::uniform_type &>(dx);
Copy link
Member

@dalg24 dalg24 May 16, 2022

Choose a reason for hiding this comment

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

Suggested change
// Test self assignment
dx = dx;
ASSERT_EQ(dx.use_count(), 1);
dx = reinterpret_cast<typename dView4::uniform_type &>(dx);
// Test self assignment
dx = dx; // copy-assignement operator
ASSERT_EQ(dx.use_count(), 1);
dx = reinterpret_cast<typename dView4::uniform_type &>(dx); // conversion constructor

Copy link
Member

@dalg24 dalg24 May 16, 2022

Choose a reason for hiding this comment

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

Actually why do we have both

template <class RT, class... RP>
KOKKOS_INLINE_FUNCTION View(
const View<RT, RP...>& rhs,
typename std::enable_if<Kokkos::Impl::ViewMapping<
traits, typename View<RT, RP...>::traits,
typename traits::specialize>::is_assignable_data_type>::type* =
nullptr)
: m_track(rhs), m_map() {
using SrcTraits = typename View<RT, RP...>::traits;
using Mapping = Kokkos::Impl::ViewMapping<traits, SrcTraits,
typename traits::specialize>;
static_assert(Mapping::is_assignable,
"Incompatible View copy construction");
Mapping::assign(m_map, rhs.m_map, rhs.m_track.m_tracker);
}

and
template <class RT, class... RP>
KOKKOS_INLINE_FUNCTION typename std::enable_if<
Kokkos::Impl::ViewMapping<
traits, typename View<RT, RP...>::traits,
typename traits::specialize>::is_assignable_data_type,
View>::type&
operator=(const View<RT, RP...>& rhs) {
using SrcTraits = typename View<RT, RP...>::traits;
using Mapping = Kokkos::Impl::ViewMapping<traits, SrcTraits,
typename traits::specialize>;
static_assert(Mapping::is_assignable, "Incompatible View copy assignment");
Mapping::assign(m_map, rhs.m_map, rhs.m_track.m_tracker);
m_track.assign(rhs);
return *this;
}

cc @nliber

@crtrott
Copy link
Member Author

crtrott commented May 16, 2022

Uh interesting:

/Users/runner/work/kokkos/kokkos/core/unit_test/TestViewAPI.hpp:1082:8: error: explicitly assigning value of variable of type 'Test::TestViewAPI::dView4' (aka 'View<T *[N1][N2][N3], DeviceType>') to itself [-Werror,-Wself-assign-overloaded]
    dx = dx;

@dalg24
Copy link
Member

dalg24 commented May 17, 2022

OpenMPTarget-ROCm build has the failure reported in #5025

[ RUN      ] openmp.scatterview
/var/jenkins/workspace/Kokkos/containers/unit_tests/TestScatterView.hpp:363: Failure
The difference between val0 and 4.0 is 4, which exceeds 1e-14 * 4.0, where
val0 evaluates to 8,
4.0 evaluates to 4, and
1e-14 * 4.0 evaluates to 4e-14.
Data differs at index 4

Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I had to double check that the reinterpret_cast was valid, which it should be as long as we don't dereference it

@crtrott crtrott merged commit bbe8b98 into kokkos:develop May 17, 2022
@crtrott crtrott deleted the view-self-assign branch May 17, 2022 18:08
@PhilMiller PhilMiller linked an issue May 17, 2022 that may be closed by this pull request
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.

View self-assignment crashes with double-free
4 participants