Repository navigation
Fix four cppia array bugs (#404, #409, #410, #411) - #438
Open
dimensionscape wants to merge 6 commits into
Open
dimensionscape wants to merge 6 commits into
dimensionscape wants to merge 6 commits into
Conversation
The interpreter took reverse() and resize() on an Array<Dynamic> but had no case for either in runVoid or runObject, so both fell through to the default, which evaluated the array and did nothing else. resize never evaluated its argument either, so a call in it was lost. They now reverse and resize the array, as they do under the JIT and in compiled code, and are typed void like the other calls that change the array. Tests: test/regression/CppiaDynamicArrayReverseResize reverses an Array<Dynamic>, then shrinks it, grows it and empties it with a counted size argument, interpreted and with the JIT, with a typed array as the control. Before, the interpreter left the array as it was and never called the size function. Fixes #409
For indexOf and lastIndexOf on an Array<Dynamic>, the JIT generated only the item, and its helpers called the array without fromIndex, so every search covered the whole array. The argument was never evaluated, so a call or an exception in it was lost too. The JIT now generates it and passes it on, null when the call leaves it out, as the interpreter does. Tests: test/regression/CppiaDynamicArraySearchFrom searches with no fromIndex, a positive, a negative and an out-of-range one, one from a counted call and one that throws, interpreted and with the JIT, with a typed array as the control. Before, the JIT found the first or last match every time, never called the function and never threw. Fixes #410
For resize() and __SetSizeExact, which cpp.NativeArray.setSize() calls, on an Array<Dynamic>, the JIT evaluated the array before its switch, as for every call but blit, and then again in their cases, into a second temporary. A call in the array expression ran twice, and when it returned another array the second time, that one was resized. Both cases now use the array evaluated and null-checked above. Tests: test/regression/CppiaDynamicArrayResizeReceiver resizes the array a counted function returns, which is a different one the second time, with resize() and with cpp.NativeArray.setSize(), interpreted and with the JIT. Before, the JIT called the function twice and resized the second array. Fixes #411
The JIT calls native helpers for blit, __SetSizeExact (which cpp.NativeArray.setSize() calls) and resize, and none of them had the exception bridge the helpers for sort, join and toString have. A range or size the array cannot take threw a C++ exception through the generated code, which has no unwind information, so the process ended instead of the script's catch running. On Windows the host exited with 0xE06D7363. The helpers now catch the exception and make it the script's pending one, and the generated code checks for it after each call, as it does after sort. For typed arrays they are three functions on ArrayBase, which does this work whatever the element type, in place of a copy for every element type and builtin: with exception handling in each copy, ArrayBuiltin.cpp needed more sections than MSVC's object file format allows without /bigobj, and the test/cppia host stopped building. Tests: test/regression/CppiaArrayHelperExceptions gives blit an out-of-range index, setSize a negative size and resize one too large to allocate, on an Array<Int> and an Array<Dynamic>, each directly in a try, interpreted and with the JIT, with a valid blit as the control. Before, the JIT run ended at the first blit. test/cppia passes interpreted and with the JIT. Fixes #404
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.
Four fixes to cppia's array builtins. Each is its own commit with its own regression test and Changes.md line, so merging with "Rebase and merge" keeps them separate on main.
reverse()andresize()on anArray<Dynamic>did nothing, because the interpreter had no case for them, andresizenever evaluated its argument.indexOfandlastIndexOfon anArray<Dynamic>ignoredfromIndexand never evaluated it.resize()andcpp.NativeArray.setSize()on anArray<Dynamic>evaluated the array expression twice, and resized the array it returned the second time.Array<Dynamic>. For typed arrays the helpers are now three functions onArrayBasein place of a copy per element type and builtin: with exception handling in each copy,ArrayBuiltin.cppneeded/bigobjon MSVC.They're one pull request because #404 and #411 change the same lines of
ArrayVirtual.cpp.Each new test runs its script interpreted and with the JIT. Its expected output is the same script compiled to C++ against this branch, and each one failed before its fix. Checked on Windows x64 with MSVC 19.64: the four new tests, nine existing cppia array and JIT regression tests, and test/cppia interpreted and with the JIT.