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

span and range checking enhancements #4125

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jan 23, 2024

  • New header span_util.h with span-related helpers:
    • make_span/make_cspan helps for converting vectors & arrays to spans when passing to templated functions.
    • spancpy, spanset, spanzero are memory-safe, range-checking substitutions for memcpy, memset when working with spans.
  • Add range check DASSERT to span access in debug mode. There were a couple spots where we were taking the address of an out-of-range value that needed to be adjusted to use a better idiom.
  • ParamValue: extra OIIO_DASSERT range check for get() method.
  • Simplify constructors by replacing &x[0] with using data(), which is always well-defined.
  • Fix typos in span.h comments

The range checking imposes no extra cost for release/optimized builds. But it might help us for CI analysis and any time we're debugging.

* New header span_util.h with span-related helpers:
    - make_span/make_cspan helps for converting vectors & arrays to
      spans when passing to templated functions.
    - spancpy, spanset, spanzero are memory-safe, range-checking
      substitutions for memcpy, memset when working with spans.
* Add range check DASSERT to span access in debug mode.
  There were a couple spots where we were taking the address of an
  out-of-range value that needed to be adjusted to use a better idiom.
* ParamValue: extra OIIO_DASSERT range check for get() method.
* Simplify constructors by replacing `&x[0]` with using `data()`, which
  is always well-defined.
* Fix typos in span.h comments

The range checking imposes no extra cost for release/optimized builds.
But it might help us for CI analysis and any time we're debugging.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 27, 2024

Does anybody have objections to this, or comments about it?

There are primarily two sets of changes in this PR:

  1. Adding debug-mode-only range checking to the indexing operators for our span<> classes. Because it's debug only, there should be no release-build performance penalty for this, but it should make it easier to identify addressing range errors when debugging and when running the testsuite in CI, especially on the sanitizer and analysis runs.

  2. Adding some new utility functions that do buffer copies with spans. We don't use these (yet), and are in a new header, so they can't possibly break anything that currently works. But I'd like to start using them where applicable, because it should give some additional protection for address range bugs.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 30, 2024

Discussed in TSC, people like spans.

@lgritz lgritz merged commit e74cd7f into AcademySoftwareFoundation:master Jan 30, 2024
25 checks passed
@lgritz lgritz deleted the lg-span branch January 30, 2024 22:41
1div0 pushed a commit to 1div0/OpenImageIO that referenced this pull request Feb 24, 2024
…4125)

* New header span_util.h with span-related helpers:
  - make_span/make_cspan helps for converting vectors & arrays to spans
when passing to templated functions.
  - spancpy, spanset, spanzero are memory-safe, range-checking
substitutions for memcpy, memset when working with spans.
* Add range check DASSERT to span access in debug mode. There were a
couple spots where we were taking the address of an out-of-range value
that needed to be adjusted to use a better idiom.
* ParamValue: extra OIIO_DASSERT range check for get() method.
* Simplify constructors by replacing `&x[0]` with using `data()`, which
is always well-defined.
* Fix typos in span.h comments

The range checking imposes no extra cost for release/optimized builds.
But it might help us for CI analysis and any time we're debugging.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Peter Kovář <peter.kovar@reflexion.tv>
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.

1 participant