Skip to content

[SYCL] Fix SegFault when accessor passed to reduction is destroyed early #3498

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 7, 2021
Merged

[SYCL] Fix SegFault when accessor passed to reduction is destroyed early #3498

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 14 additions & 28 deletions sycl/include/CL/sycl/ONEAPI/reduction.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,30 +508,24 @@ class reduction_impl : private reduction_impl_base {
}

/// Constructs reduction_impl when the identity value is statically known.
// Note that aliasing constructor was used to initialize MRWAcc to avoid
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "aliasing constructor" was not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of using aliasing constructor was to give the pointer to user's accessor to shared_ptr, but not release memory (i.e. user's accessor) when that shared_ptr is destroyed, at least that is how I understood it when wrote that code.

That was wrong approach for cases where user's accessor is destroyed before the accessor is needed in reduction computation.
At this line: https://github.com/v-klochkov/llvm-test-suite/blob/public_vklochkov_reduction_ctor_acc_destroyed_early/SYCL/Reduction/reduction_nd_lambda.cpp#L41
MRWAcc shared_ptr holded a pointer to released memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks for explanation

// destruction of the object referenced by the parameter Acc.
template <
typename _T = T,
enable_if_t<IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(rw_accessor_type &Acc)
: MRWAcc(shared_ptr_class<rw_accessor_type>(
shared_ptr_class<rw_accessor_type>{}, &Acc)),
MIdentity(getIdentity()), InitializeToIdentity(false) {
: MRWAcc(new rw_accessor_type(Acc)), MIdentity(getIdentity()),
InitializeToIdentity(false) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
}

/// Constructs reduction_impl when the identity value is statically known.
// Note that aliasing constructor was used to initialize MDWAcc to avoid
// destruction of the object referenced by the parameter Acc.
template <
typename _T = T,
enable_if_t<IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(dw_accessor_type &Acc)
: MDWAcc(shared_ptr_class<dw_accessor_type>(
shared_ptr_class<dw_accessor_type>{}, &Acc)),
MIdentity(getIdentity()), InitializeToIdentity(true) {
: MDWAcc(new dw_accessor_type(Acc)), MIdentity(getIdentity()),
InitializeToIdentity(true) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
Expand Down Expand Up @@ -567,15 +561,12 @@ class reduction_impl : private reduction_impl_base {

/// Constructs reduction_impl when the identity value is statically known,
/// and user still passed the identity value.
// Note that aliasing constructor was used to initialize MRWAcc to avoid
// destruction of the object referenced by the parameter Acc.
template <
typename _T = T,
enable_if_t<IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(rw_accessor_type &Acc, const T & /*Identity*/, BinaryOperation)
: MRWAcc(shared_ptr_class<rw_accessor_type>(
shared_ptr_class<rw_accessor_type>{}, &Acc)),
MIdentity(getIdentity()), InitializeToIdentity(false) {
: MRWAcc(new rw_accessor_type(Acc)), MIdentity(getIdentity()),
InitializeToIdentity(false) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
Expand All @@ -592,13 +583,14 @@ class reduction_impl : private reduction_impl_base {
// list of known operations does not break the existing programs.
}

/// Constructs reduction_impl when the identity value is statically known,
/// and user still passed the identity value.
template <
typename _T = T,
enable_if_t<IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(dw_accessor_type &Acc, const T & /*Identity*/, BinaryOperation)
: MDWAcc(shared_ptr_class<dw_accessor_type>(
shared_ptr_class<dw_accessor_type>{}, &Acc)),
MIdentity(getIdentity()), InitializeToIdentity(true) {
: MDWAcc(new dw_accessor_type(Acc)), MIdentity(getIdentity()),
InitializeToIdentity(true) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
Expand Down Expand Up @@ -632,30 +624,24 @@ class reduction_impl : private reduction_impl_base {
}

/// Constructs reduction_impl when the identity value is unknown.
// Note that aliasing constructor was used to initialize MRWAcc to avoid
// destruction of the object referenced by the parameter Acc.
template <
typename _T = T,
enable_if_t<!IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(rw_accessor_type &Acc, const T &Identity, BinaryOperation BOp)
: MRWAcc(shared_ptr_class<rw_accessor_type>(
shared_ptr_class<rw_accessor_type>{}, &Acc)),
MIdentity(Identity), MBinaryOp(BOp), InitializeToIdentity(false) {
: MRWAcc(new rw_accessor_type(Acc)), MIdentity(Identity), MBinaryOp(BOp),
InitializeToIdentity(false) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
}

/// Constructs reduction_impl when the identity value is unknown.
// Note that aliasing constructor was used to initialize MDWAcc to avoid
// destruction of the object referenced by the parameter Acc.
template <
typename _T = T,
enable_if_t<!IsKnownIdentityOp<_T, BinaryOperation>::value> * = nullptr>
reduction_impl(dw_accessor_type &Acc, const T &Identity, BinaryOperation BOp)
: MDWAcc(shared_ptr_class<dw_accessor_type>(
shared_ptr_class<dw_accessor_type>{}, &Acc)),
MIdentity(Identity), MBinaryOp(BOp), InitializeToIdentity(true) {
: MDWAcc(new dw_accessor_type(Acc)), MIdentity(Identity), MBinaryOp(BOp),
InitializeToIdentity(true) {
if (Acc.get_count() != 1)
throw sycl::runtime_error("Reduction variable must be a scalar.",
PI_INVALID_VALUE);
Expand Down