Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable tests failing due to missing array ctor and lbound index checks. #35008

Merged

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented Apr 15, 2020

Fix mono/mono#19494 adds missing array ctor needed by several tests currently failing as described by #34068.

This PR fixes an additional problem hit by the IL array tests using an lbound != 0. Current Mono codegen for Set/Get/Address array methods didn't adjust index with regards to lbound causing failures in several of the array tests. NOTE, this scenario should only be hit when directly calling Set/Get/Address using IL on an array with an lbound != 0. It shouldn't affect regular szarray's at all since that code paths should never be used and the codegen currently looks up the lbounds unconditionally in the array, so if that code path ever gets hit by a szarray, it will imdediate crash, so shouldn't be causing any performance impact on szarrays. There is also an additional check on the array type as well as an additional precaution, so only arrays of type MONO_TYPE_ARRAY, hitting codegen for Set/Get/Address array method calls will adjust index based on lbound.

This PR fix the lbound index codegen for that specific code path and re-eanbles the tests currently listed in #34068.

@ghost
Copy link

ghost commented Apr 15, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@danmoseley
Copy link
Member

Poor bot, I guess it saw ".targets"

@lateralusX
Copy link
Member Author

Depending on #34054 to be merged before CoreCLR test suites will run using Mono on CI.

@lateralusX lateralusX added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 16, 2020
@lateralusX
Copy link
Member Author

lateralusX commented May 4, 2020

@naricc CoreCLR tests are now running on Mono runtime, correct? If so I guess it's time to rebase this PR and validate the results when reenable the array ctor tests on CI.

@naricc
Copy link
Contributor

naricc commented May 4, 2020

@lateralusX Yes, the runtime tests (I'm trying to stop calling them the CoreCLR tests, since they now are testing multiple runtimes and it gets confusing :) ) are now in CI. You will see them as "Mono Pri0 Runtime Tests Run OSX x64 release". I am working on standing up CI lanes for other architectures/modes (like interpreter).

@lambdageek
Copy link
Member

@naricc could you resolve the conflicts in this PR

@naricc
Copy link
Contributor

naricc commented Jun 22, 2020

@lambdageek Ok; on my list.

@naricc naricc self-assigned this Jun 22, 2020
@lambdageek lambdageek added this to the 5.0.0 milestone Jun 23, 2020
@lambdageek lambdageek added test-enhancement Improvements of test source code and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Jun 23, 2020
@stephentoub
Copy link
Member

There are conflicts. Mind rebasing? Thanks.

@lateralusX
Copy link
Member Author

Investigate this a little further, we now get passed the error fixed by mono/mono#19494, but several of these tests also use constructions to test lbound != 0 and then access the array using indexes inline with the new low and upper bounds. On Mono this will fail with 1 dim arrays since we don't adjust index based on lbound != 0. This needs to be fixed before enable the tests or we will still get failures.

@stephentoub
Copy link
Member

Thanks. Is there an open issue tracking that?

@stephentoub
Copy link
Member

(And should this be closed in the interim?)

@lateralusX
Copy link
Member Author

lateralusX commented Jul 9, 2020

I believe the only one we currently have is the one referenced in this PR, #34068, but that highlights another problem that is solved (missing constructor). I will look into a potential fix for the other things not working in the tests, but depending on the outcome of that fix and perf impact, I could add that fix into this PR and re-enable tests as part of complete fix. If we come to the conclusion that the fix adds to much perf overhead, I believe we also decided that we won't fix the failing tests, so then we can go ahead and close both this PR and the original issue.

@lateralusX
Copy link
Member Author

I will rebase this PR, add additional fix into this PR and re-enable all the tests tomorrow morning.

@lateralusX
Copy link
Member Author

@naricc I take over this PR again since it needs some additional fixes before all tests pass (not enough to just enable them).

@lateralusX lateralusX assigned lateralusX and unassigned naricc Jul 9, 2020
@lateralusX lateralusX force-pushed the lateralusX/enable-issue-34068-tests branch from a461802 to 6c20ddf Compare July 10, 2020 11:35
@lateralusX lateralusX changed the title Enable tests previously failing due to missing array ctor. Enable tests failing due to missing array ctor and lbound index checks. Jul 10, 2020
@lateralusX lateralusX added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 10, 2020
@lateralusX
Copy link
Member Author

lateralusX commented Jul 10, 2020

Tests pass on Windows x64, so expected at least OSX x64 to get same results, but test runs looks like the additional codegen is not hit, since failures looks close to what happens when the lbound is not adjusted. Need to investigate the tests on none Windows platforms.

@lateralusX
Copy link
Member Author

Failures was not in JIT, but interpreter config, updated with similar adjustments done to JIT in interpreter.

@lateralusX
Copy link
Member Author

There is also a missing OP code on arm64 causing issue to convert float -> int, adding simliar conversion for OP_FCONV_TO_I as done on amd64, arm and x86.

@lateralusX lateralusX force-pushed the lateralusX/enable-issue-34068-tests branch from dd77b80 to 0778896 Compare July 13, 2020 08:42
@lateralusX lateralusX requested a review from BrzVlad as a code owner July 13, 2020 08:42
@lateralusX
Copy link
Member Author

Down to 3 failures, only interpreter lane on _il_relgcarr, _il_relint32_m1, _il_dbgint32_m1, so these three cases must hit some other code path in interpreter compared to rest, will investigate.

@lateralusX
Copy link
Member Author

Down to one failing tests in mono/mono, apparently we take one internal shortcut when creating a MONO_TYPE_ARRAY with lower bound of 0 and don't create the runtime lbound struct, so we need to add condition in codegen for that case.

@lateralusX lateralusX force-pushed the lateralusX/enable-issue-34068-tests branch from 65fb519 to fd16a62 Compare July 17, 2020 13:40
Enable tests failing due to missing array ctor and lbound index checks.

Fix interpreter Get/Set/Address using lbound.

Add missing OP_FCONV_TO_I on Arm64, causing test errors.

Fix interpreter arrray index and lower bound when using negative values.

Add branch checking array->bounds for NULL.
@lateralusX lateralusX force-pushed the lateralusX/enable-issue-34068-tests branch from fd16a62 to f7227ec Compare August 18, 2020 06:52
@lateralusX lateralusX merged commit 3aced82 into dotnet:master Aug 18, 2020
imhameed added a commit to imhameed/mono that referenced this pull request Nov 4, 2020
imhameed added a commit to mono/mono that referenced this pull request Nov 6, 2020
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Nov 22, 2020
Context: dotnet/runtime#35008 (comment)
Context: mono/mono#15418
Context: mono/mono#16763
Context: mono/mono#20490
Context: mono/mono#20533
Context: xamarin/xamarin-macios#9949

Changes: mono/mono@be2226b...ac59637

  * mono/mono@ac596375c76: Add support for OP_FCONV_TO_I to mini-arm64.c. (#20548)
  * mono/mono@392fe5b87b2: [2020-02][watchOS] Add simwatch64 support (#20552)
  * mono/mono@a22ed3f094e: Fix potential crash for Encoder.Convert (#20522)
  * mono/mono@970783731fc: Bump to F# 5.0 (#20511)
  * mono/mono@32ab5066f72: Bump msbuild to fix a build issue
  * mono/mono@93a7fe77e8e: Ensure special static slots respect alignment. (#20506)
  * mono/mono@3db5b358413: [debugger] Switch to GC Unsafe in signal handler callbacks (#20495)
  * mono/mono@af315f44c40: [2020-02][corlib] ThreadAbortException protection for ArraySortHelper (#20468)
  * mono/mono@ca11fb0fd81: [2020-02] Bump ikvm-fork to include mono/ikvm-fork#20 (#20452)
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants