Add a smoke test for non-empty nullptr Span#107444
Merged
akien-mga merged 1 commit intogodotengine:masterfrom Jun 12, 2025
Merged
Add a smoke test for non-empty nullptr Span#107444akien-mga merged 1 commit intogodotengine:masterfrom
nullptr Span#107444akien-mga merged 1 commit intogodotengine:masterfrom
Conversation
08bb1b9 to
2ef2ea4
Compare
…`nullptr` `Span`.
2ef2ea4 to
2d0ff97
Compare
lawnjelly
reviewed
Jun 12, 2025
| _len = 0; | ||
| } | ||
| #endif | ||
| } |
Member
There was a problem hiding this comment.
Could we do this, or is too risky of having things compile in release but not debug? (and vice versa) 🤔
The CI checks should be pretty good at catching any problems as they do editor and template builds.
#ifdef DEBUG_ENABLED
_FORCE_INLINE_ Span(const T *p_ptr, uint64_t p_len) :
_ptr(p_ptr), _len(p_len) {
// TODO In c++20, make this check run only in non-consteval, and make this constructor constexpr.
if (_ptr == nullptr && _len > 0) {
ERR_PRINT("Internal bug, please report: Span was created from nullptr with size > 0. Recovering by using size = 0.");
_len = 0;
}
}
#else
_FORCE_INLINE_ constexpr Span(const T *p_ptr, uint64_t p_len) :
_ptr(p_ptr), _len(p_len) {}
#endif
Member
Author
There was a problem hiding this comment.
That would work, but i think since it's effectively constexpr anyway, it should be compiled as such with or without the constexpr modifier. The only difference being that we won't be able to use it in constexpr contexts, which is also the case with separate DEBUG and RELEASE definitions.
lawnjelly
approved these changes
Jun 12, 2025
Member
|
Thanks! |
This file contains hidden or 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
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 above bug occurred because a
Spanwas created fromnullptrwith asize > 0. This is incorrect, because it's promising access to a memory region that does not exist.We can catch this, at least for the
nullptrcase, by adding a smoke test. This should help us catch such bugs early in the future. The above bug could only have occurred inDEBUGbuilds, so it would have been prevented by this test.The test is pretty cheap, so it shouldn't create performance problems for
DEBUG. I'd still hesitate to add it toRELEASEbecause correct code should not need it, and in hot loops, it may actually prevent vectorization. Plus, setting_len = 0is unlikely to be the correct fix anyway (though it should be better than nothing).Note: Unfortunately, this change requires the function to be non-
constexpr, becauseERR_PRINTisn'tconstexpr. This can be fixed in c++20 withstd::is_constant_evaluated. Fortunately, we do not use this property yet, so for Godot 4.5 we can give it up. In 4.6, we're planning to upgrade to C++20, so we can re-addconstexprthen.