scope.c - rework SSGROW() and SSCHECK() macros and undelying functions #20824
Conversation
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
This was referenced Feb 19, 2023
We have a bug where we can overflow the save-stack. This tests for it in a TODO test. The next patch will fix it. Note the test will only fail in debugging as it requires the assert() to be compiled in.
Prior to this patch SSCHECK() took a "needs" parameter, but did not actually guarantee that the stack would be sufficiently large to accomodate that many elements after the call. This was quite misleading. Especially as SSGROW() would not do geometric preallocation, but SSCHECK() would, so much of the time SSCHECK() would appear to be a better choice, but not always. This patch makes it so SSCHECK() is an alias for SSGROW(), and it makes it so that SSGROW() also geometrically overallocates. The underlying function that used to implement SSCHECK() savestack_grow() now calls the savestack_grow_cnt() which has always implemented SSGROW(). Anything in the internals that used to call SSCHECK() now calls SSGROW() instead. At the same time the preallocation has been made a little bit more aggressive, ensuring that we always allocate at least SS_MAXPUSH elements on top of what was requested as part of the "known" size of the stack, and additional SS_MAXPUSH elements which are not part of the "known" size of the stack. This "hidden extra" is used to simply some of the other macros which are used a lot internally. (I have beefed up the comment explaining this as well.) This fixes GH Issue #20826
demerphq
force-pushed
the
yves/unify_SSCHECK_and_SSGROW
branch
from
81d9b32 to
217acab
Compare
|
Merging as this fixes a bug I dont think we should let loose in a released perl. |
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.
Prior to this patch SSCHECK() took a "needs" parameter, but did not actually guarantee that the stack would be sufficiently large to accomodate that many elements after the call. This was quite misleading. Especially as SSGROW() would not do geometric preallocation, but SSCHECK() would, so much of the time SSCHECK() would appear to be a better choice, but not always.
This patch makes it so SSCHECK() is an alias for SSGROW(), and it makes it so that SSGROW() also geometrically overallocates. The underlying function that used to implement SSCHECK() savestack_grow() now calls the savestack_grow_cnt() which has always implemented SSGROW(). Anything in the internals that used to call SSCHECK() now calls SSGROW() instead.
At the same time the preallocation has been made a little bit more aggressive, ensuring that we always allocate at least SS_MAXPUSH elements on top of what was requested as part of the "known" size of the stack, and additional SS_MAXPUSH elements which are not part of the "known" size of the stack. This "hidden extra" is used to simply some of the other macros which are used a lot internally. (I have beefed up the comment explaining this as well.)
See #20530 where Dave M points out the problem with this logic. Currently I don't think we have a good way to test for the error that he showed so unfortunately this patch lacks a test.