[PROTOTYPE] Add unused temporary storage to single work-group scan to fix use-after free error #1712
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -681,6 +681,10 @@ __parallel_transform_scan_single_group(oneapi::dpl::__internal::__device_backend | |||
| // Specialization for devices that have a max work-group size of 1024 | |||
| constexpr ::std::uint16_t __targeted_wg_size = 1024; | |||
|
|
|||
| using _ValueType = typename _InitType::__value_type; | |||
| using _TempStorage = __result_and_scratch_storage<std::decay_t<_ExecutionPolicy>, _ValueType>; | |||
| _TempStorage __result_and_scratch{__exec, 0}; | |||
There was a problem hiding this comment.
Can we name this __dummy_result_and_scratch or something similar to draw attention to why this is here but unused in practice?
Also can we add a comment explaining the purpose of this dummy scratch space?
There was a problem hiding this comment.
I renamed it and added a comment explaining why it's here.
| @@ -699,17 +703,17 @@ __parallel_transform_scan_single_group(oneapi::dpl::__internal::__device_backend | |||
| ::std::integral_constant<::std::uint16_t, __num_elems_per_item>, _BinaryOperation, | |||
| /* _IsFullGroup= */ std::true_type, _Inclusive, _CustomName>>>()( | |||
| ::std::forward<_ExecutionPolicy>(__exec), std::forward<_InRng>(__in_rng), | |||
| std::forward<_OutRng>(__out_rng), __n, __init, __binary_op, __unary_op); | |||
| std::forward<_OutRng>(__out_rng), __n, __init, __binary_op, __unary_op, __result_and_scratch); | |||
There was a problem hiding this comment.
Do we need to pass this scratch space in only to return it?
Perhaps it is more clear if we instead return an event from the submitter, and construct the future here with the dummy variable. It would make it more clear that it goes unused, and minimize the changes required.
There was a problem hiding this comment.
Good point. I changed the submitters to return an event and constructed the future in the outer context instead.
|
LGTM (from phone) |
adamfidel
added a commit
that referenced
this pull request
Jul 24, 2024
… fix use-after free error (#1712)
mmichel11
pushed a commit
that referenced
this pull request
Jul 31, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 5, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 6, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 6, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 7, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 7, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 8, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 8, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 8, 2024
… fix use-after free error (#1712)
danhoeflinger
pushed a commit
that referenced
this pull request
Aug 14, 2024
… fix use-after free error (#1712)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were throwing away the result and scratch container inside of the future in
__parallel_transform_scanbecause we needed to return a future without storage. This was causing a use-after free error because the scratch storage was freed before it was used in the kernel.