FIX: issues #3369, #4088, #4087, #4085, #4083, #4078, #4913

[hiiamboris]

Fixes #3369 , fixes #4078 , fixes #4083 , fixes #4085 , fixes #4087 , fixes #4088 , fixes #4100 , fixes #4913

This PR fixes numerous crashes in series-related functions, and includes a total rewrite of change and take. Lots of tests included.

Series funcs behavior now follows the one I outlined in red/red in all edge cases. How it all works is seen from the tests, so I'm not duping it here.

next, skip, at I do not plan to modify until we agree on some sort of series model.
take/last/part behavior is unchanged, only fixed, although I'm not happy with it (reasons)

Status

This PR is ready for review, though a bit more work is required desirable:

• move func has no tests at all, and it's very unintuitive, I'd love to rewrite it as well (see unexpected behavior and access violation of MOVE/PART applied to the same series #3527 ); no tests for it so far; alter has no tests either
• I didn't test select/pick/random/min/max yet
• I didn't run tests except series-test.red yet
• All tests succeed when run from the console, but some heisenbug still remains, causing run-all-interp.red (when compiled only) to stop just after fun-12 test, executing only 1687 of over 10k tests, ignorantly reporting a false success

P.S. Added memory protection (see below)

[qtxie]

Thanks for diving into those complicated issue! 👍 That's really helpful. I'll review the code ASAP.

[hiiamboris]

1687 of 11000+ tests being executed turns out to be a GC bug. Insert recycle/off after this line and tests finish correctly.

[hiiamboris]

Here's a candy in commit 9a857d0. A taste of design-by-contract for R/S, that so far has helped me to:

• find 3 memory corruption bugs
• ascertain that the other code under supervision is working correctly

Rationale

While we can easily tell that a four lines long function does what's expected, a function 50 lines long is usually a black box. We can only trust that it works, but we do not know that. For these longer functions it is imperative we have tools that can verify the algorithm. As a bonus we will spend less time double checking our own code.

Sources of risk

• bulk memory writes (copy-memory, move-memory)
• writes to pointers in a loop (p/1:, p/x:, p/value: patterns)
• delegated writes (one function calls another, or a macro, with a pointer, and that function/macro doesn't know the limits of the pointed region)

Usage

1. Declaration
   To check memory writes one has to define regions where those write are allowed.
   Usually one has a series one wishes to modify, or a simple byte buffer. One declares that one is gonna modify those and nothing else like this:
   MEMGUARD_ADD(series) - adds any red-series!-compatible type buffer as a write target
   MEMGUARD_ADDNODE(node) - adds a buffer referred to by a node as a write target
   MEMGUARD_ADDRANGE(start bytes) - adds a fixed (non-relocatable) buffer as a write target
   MEMGUARD_ADDRANGE2(start end) - same
   Writes outside of these regions will be reported.

2. Verification

• move-memory and copy-memory are checked automatically. To prevent it and allow an unchecked write one should write:
  MEMGUARD_UNCHECKED copy-memory <dst> <src> <size>
• To verify pointer access an explicit check is required:
  MEMGUARD_RANGECHK(start bytes) - verifies a region
  MEMGUARD_PCHK(pointer) - verifies that pointer/value is safe to write (also works for structures, like cell!)

3. Initialization & finalization
   Every function should normally be responsible for it's own writes. Other functions that it may call should usually take care of their writes themselves. To divide the responsibility a stack of regions is built.

• One starts to define a new set of regions by calling MEMGUARD_MARK. It should come before any range declarations, and nullifies the previous ones.
• Upon exiting, a function is responsible to call MEMGUARD_BACK. It will restore the previously defined ranges so that the calling function may continue. Watch out for return, exit, fire, ERR_* and --NOT_IMPLEMENTED-- macros, as well as functions that can throw exceptions.
• To avoid an accidental replacement of the region (with a misplaced MARK, or a forgotten BACK), these two functions communicate a special token (set by MARK and checked by BACK). For that reason one should add a special variable into it's locals:
  func [ ... /locals var.. [type!] ... MEMGUARD_TOKEN ][ ... code ... ]

4. Design remarks

• This tool has no effect outside of debug mode.
• Code outside of initialization/finalization scope is unaffected.
• Stack segment is ignored.
• Examples can be found in the PR code.

Hope you will find this useful ☻

[qtxie]

There is a PR for this GC bug.

[hiiamboris]

@qtxie oh! I thought it was merged :)
Should I apply those commits here as well?

[qtxie]

Better to cherry-pick that commit in your branch to see if it will fix the issue.

[hiiamboris]

Yes, works great with those commits.

[hiiamboris]

reminder: #4788 lists some bugs of this PR too

also need to deal with crash on

s: []
o: make deep-reactor! [
	on-deep-change*: func spec: [owner word target action new index part] [
		print ["Whoops!"]
	]
	x: s
]
change s 1

likely just need to mark memory access in dyn-print

[greggirwin]

@dockimbel is there anything specific holding up the merge on this PR?

[greggirwin]

It has conflicts now, but I don't want @hiiamboris to spend time on that if it's still not going to get merged for some other reason.