Support a value stack with over 2**31 entries on 64-bit machines #20990
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
demerphq
approved these changes
Mar 30, 2023
|
On Wed, Mar 29, 2023 at 06:25:44PM -0700, Tony Cook wrote:
This series changes the mark stack and various macros to use SSIze_t instead of I32 for marks.
I strongly suspect that many of the changes in this series of commits is
going to cause a merge conflict with something in the 66 commits (so
far) of my 'make the stack refcounted' branch.
Would it possible to shelve this work and revisit after most of the
refcount work is finished and merged?
…--
A problem shared is a problem doubled.
|
|
I don't intend to apply it until 5.39, I expect the changes could cause some noise downstream (mostly C conversion warnings if anyone enables them). I'll rebase on top of the refcounted stack changes once they're applied. |
tonycoz
added
the
defer-next-dev
|
On Sun, Apr 02, 2023 at 05:26:31PM -0700, Tony Cook wrote:
I'll rebase on top of the refcounted stack changes once they're applied.
Thanks!
…--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"
|
|
Made this draft to ensure it isn't merged until refcounted stack it merged. |
|
@iabyn I think you should merge your refcounted stack changes asap. |
tonycoz
removed
the
defer-next-dev
tonycoz
force-pushed
the
20917-mark-of-an-appropriate-size
branch
from
c0f8f7a to
4352e5e
Compare
tonycoz
force-pushed
the
20917-mark-of-an-appropriate-size
branch
from
4352e5e to
a13ffe9
Compare
tonycoz
force-pushed
the
20917-mark-of-an-appropriate-size
branch
from
a13ffe9 to
fd8c051
Compare
The basic test passes Works towards Perl#20917
Done by updating the definitions for AX and items to SSize_t The bootcheck macros will require a bit more work, and need a separate test.
I'm not entirely happy with this, since we're changing the return type and the types of the items and ax values passed via va_args. But an excuse: the return and the fetching of ax and items via va_args both happen after we've validated the handshake key, which would prevent the va_arg() fetches and the return. But but: this is fine for va_args, but purely the call to the function with the different return type is technically undefined behaviour. I'm not sure it's worth trying to workaround that, since it will require code duplication for a rare case, that I expect to just work on the platforms involved (64-bit platforms).
The tests here move/set a lot of memory, and this makes them slow. If we want to debug a particular test, running them all wastes a lot of time, so allow particular tests to be run.
OP_LIST is optimised away in many cases, the hard part was creating the test. This ended up being fixed by one of Dave's rc_stack patches, but leave the test in.
Not really related to the stack, but this came up when searching for I32 mis-use. This didn't crash, instead the loop exited early.
I can't write a test for this, I'd expect it to use at least 96GB to test.
This calls repeatcpy() which took an I32 for the size of the object to copy, change this to SSize_t, and update the list code in pp_repeat to only test if the new allocation would fail.
No real way to test this. i looks suspicious too, but it can't go above 15.
Since pp_mapwhile mortalcopy()s many SVs I don't have enough memory to test this usefully, so no tests.
I do think it's unlikely apply() would be called with >2G filenames or handles, but it's a limit. The test is slow, very slow.
index into the parameter list.
eval_sv() had two problems, it saved the starting stack position in an I32, so a call with a deep stack would save a negative or otherwise invalid depth, and the return value was an I32, so if the eval returned a large list the return count would overflow.
call_sv() used I32 for a saved mark and the return value. The XS::APItest call_sv() wrapper used an I32 index to reposition the argument list supplied to call_sv(). I haven't created additional tests for call_argv(), call_pv() nor call_method(), these simply pass the return value on, I don't think it worth the extra tests.
tonycoz
force-pushed
the
20917-mark-of-an-appropriate-size
branch
from
fd8c051 to
ac78ddd
Compare
|
Applied manually |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This series changes the mark stack and various macros to use SSIze_t instead of I32 for marks.
It also fixes several places where perl uses an I32 as an index into the stack, or an index into a list on the stack, or as a count of items returned on the stack.
It doesn't attempt to fix all I32 problems, but I do have a little list I'll work through of potential problems to check out.
Fixes #20917