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

Fix LWG Issue 4060 (fix submdspan to prevent invalid pointer creation) #323

Merged
merged 15 commits into from
May 3, 2024

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Apr 2, 2024

Link to issue: https://cplusplus.github.io/LWG/issue4060

  1. Fix submdspan to prevent invalid pointer creation
  2. Implement bounds checking for the various layouts, so we can actually detect the issue

I aimed for C++14 backwards compatibility. Item (2) above is not strictly necessary to fix the issue, but it does help with finding the issue in the first place.

Bounds checking is only enabled in a debug build (if NDEBUG is not defined). It involves some fold expressions over assert expressions. I'm not attached to the use of assert for reporting errors.

mhoemmen added 9 commits April 2, 2024 13:05
LWG Issue 4060 ("submdspan preconditions do not forbid creating
invalid pointer") currently fails, because submdspan does not
correctly handle the case where first_ of one or more slices is out of
bounds.

This commit adds two tests taken straight from the Issue.
As expected, one test passes and the other test fails.
However, both tests trigger undefined behavior
for the layout mapping.
... in favor of macros.  In theory, this would help us backport
submdspan to C++14.
@mhoemmen mhoemmen requested review from nmm0 and crtrott April 2, 2024 21:30
mhoemmen added 2 commits April 4, 2024 09:23
...thus following the pattern of other uses of assert
in the same header file.
* Add MDSPAN_INLINE_FUNCTION to bounds-checking functions
* Pass indices by value to bounds-checking functions
* Don't use std::forward for slices (use const ref instead)
* Use required_span_size() in test
@mhoemmen mhoemmen force-pushed the fix-LWG-Issue-4060 branch from 59779f1 to fab6a90 Compare April 4, 2024 15:51
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

LGTM (after you fix the bool_constant)

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@crtrott crtrott merged commit a9c54cc into kokkos:stable May 3, 2024
10 checks passed
@mhoemmen mhoemmen deleted the fix-LWG-Issue-4060 branch May 5, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants