This PR fixes a few problems reported by Clang's Undefined Behavior sanitizer #2139
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
Instead of reinterpret_casting to an possibly unrelated promise_base, this patch uses the common base_lco base for the managed_promise shim.
the promise is allocated as promise<void>, however the continuation tried to trigger a promise<unused_type>, this triggers UB and has been fixed in this patch. Flyby: Some cleanup of basic_action.hpp and continuation.hpp
This patch fixes a problem when the result type specified for packaged_action does not match the action's local result type. This led to the continuation calling the set_value_nonvirt function on an object of wrong type.
| struct sync_invoke | ||
| { | ||
| template <typename IdOrPolicy, typename ...Ts> | ||
| HPX_FORCEINLINE static LocalResult call( | ||
| HPX_FORCEINLINE static local_result_type call( | ||
| boost::mpl::false_, launch policy, |
There was a problem hiding this comment.
Might have been a good idea to replace boost::mpl::true_/false_ here as well - all the rest of the file has been switched to <type_traits>.
There was a problem hiding this comment.
The Problem here is that is_future returns a boost::mpl type. I wanted to
avoid changing too much unrelated stuff in one PR.
…uture Removed special handling of typed_continuation<([shared_]future<R>> and replaced it with free function handling of future return types.
sithhell
force-pushed
the
ubsan
branch
3 times, most recently
from
749aa9d
to
c5b4798
Compare
|
All comments have been addressed now. This PR lets the HPX unit and regression tests run with UBsan without failure. |
| Component::heap_type::free(cv, 1); | ||
| return naming::invalid_gid; | ||
| } | ||
| Component *c = static_cast<Component *>(cv); |
There was a problem hiding this comment.
This function is throwing an exception if creation goes wrong. I don't think we should silently return invalid_gid in case of an error in the constructor.
Flyby: replaced all actions::extract_action with traits::extract_action
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.
The problems included:
Removed special handling of typed_continuation<([shared_]future> and
replaced it with free function handling of future return types.
This patch fixes a problem when the result type specified for packaged_action
does not match the action's local result type. This led to the continuation
calling the set_value_nonvirt function on an object of wrong type.
the promise is allocated as promise, however the continuation
tried to trigger a promise<unused_type>, this triggers UB and has
been fixed in this patch.
Flyby: Some cleanup of basic_action.hpp and continuation.hpp