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.

Sign up for GitHub

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

Jump to bottom

[mono][jit] Emit a null check when storing to valuetype fields. #82663

Merged
merged 3 commits into from
Apr 29, 2023
Merged

[mono][jit] Emit a null check when storing to valuetype fields. #82663

merged 3 commits into from
Apr 29, 2023

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Feb 25, 2023

Fixes #82535.

@ghost ghost assigned vargaz Feb 25, 2023
@vargaz
Copy link
Contributor Author

vargaz commented Feb 27, 2023

Still needs a regression test.

@vargaz vargaz force-pushed the store_membase_null_check branch from 3e2401c to 3149a6d Compare February 28, 2023 14:06
This was referenced Feb 28, 2023
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Feb 28, 2023
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Mar 1, 2023

The new test seems to crash on the clr ?

   JIT/Regression/JitBlue/Runtime_82535/Runtime_82535/Runtime_82535.sh [FAIL]
      [createdump] Invalid process id: task_for_pid(4941) FAILED (os/kern) failure (5)
      [createdump] This failure may be because createdump or the application is not properly signed and entitled.
      [createdump] Failure took 0ms
      [createdump] waitpid() returned successfully (wstatus 0000ff00)
      /private/tmp/helix/working/B0370906/w/B5A80970/e/JIT/Regression/JitBlue/Runtime_82535/Runtime_82535/Runtime_82535.sh: line 425:  4941 Segmentation fault: 11  (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"

@build-analysis build-analysis bot mentioned this pull request Mar 1, 2023
Open
@lambdageek
Copy link
Member

lambdageek commented Mar 3, 2023 •
edited
Loading

ping @mangod9 seems like there's two issues uncovered by the new test:

  1. some kind of segfault instead of a NullReferenceException - perhaps just in checked builds
  2. some kind of problem with coredump on osx

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-82663-merge-d7b5be7d292f490986/JIT.Regression/1/console.40974b4f.log?helixlogtype=result

https://dev.azure.com/dnceng-public/public/_build/results?buildId=187998&view=logs&jobId=d660e68b-c00d-5466-3901-f111ca4fe7bd&j=d660e68b-c00d-5466-3901-f111ca4fe7bd&t=0b07bb4f-f6f8-5a4c-8b40-7d826f2cf02c

@mangod9
Copy link
Member

mangod9 commented Mar 4, 2023

Is this a consistent failure on linux x64 Debug builds? Adding @janvorli since we had fixed something similar in 7.

@lewing lewing added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 5, 2023
@ghost
Copy link

ghost commented Mar 5, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #82535.

Author: vargaz
Assignees: vargaz
Labels:

area-CodeGen-coreclr, area-Codegen-JIT-mono
Milestone: -

@lewing lewing closed this Mar 10, 2023
@lewing lewing reopened this Mar 10, 2023
This was referenced Mar 11, 2023
Closed
Closed
@marek-safar marek-safar removed the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@vargaz vargaz force-pushed the store_membase_null_check branch from 72080c6 to 03f8a97 Compare March 28, 2023 15:22
@build-analysis build-analysis bot mentioned this pull request Mar 28, 2023
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Apr 10, 2023

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz force-pushed the store_membase_null_check branch from 03f8a97 to bba19c0 Compare April 10, 2023 04:56
@build-analysis build-analysis bot mentioned this pull request Apr 19, 2023
Closed
@vargaz vargaz force-pushed the store_membase_null_check branch from bba19c0 to e9adfde Compare April 28, 2023 19:06
@build-analysis build-analysis bot mentioned this pull request Apr 28, 2023
Closed
@vargaz vargaz force-pushed the store_membase_null_check branch from e9adfde to 917feae Compare April 28, 2023 22:37
@vargaz vargaz merged commit 6bb5449 into dotnet:main Apr 29, 2023
@vargaz vargaz deleted the store_membase_null_check branch April 29, 2023 04:23
MichalStrehovsky
MichalStrehovsky reviewed May 1, 2023
View reviewed changes
src/tests/issues.targets
@@ -66,6 +66,9 @@
<ExcludeList Include="$(XunitTestBinBase)/GC/Regressions/Github/Runtime_76219/Runtime_76219/*">
<Issue>https://github.com/dotnet/runtime/issues/78899</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_82535/*">
<Issue>https://github.com/dotnet/runtime/pull/82663</Issue>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a tracking issue for the CoreCLR problem with this test? We don't typically put pull requests into the Issue field because Mono pull requests are not CoreCLR tracking issues that someone would look at.

Cc @mangod9 @janvorli @dotnet/jit-contrib

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make one.

MichalStrehovsky
MichalStrehovsky reviewed May 1, 2023
View reviewed changes
src/tests/JIT/Regression/JitBlue/Runtime_82535/Runtime_82535.csproj
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
<RequiresProcessIsolation>true</RequiresProcessIsolation>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this require process isolation? RequiresProcessIsolation adds extra costs that the JIT team is actively working on reducing right now. Throwing and catching a nullref doesn't sounds like something that would need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It caused a process crash on CI, which doesn't seem to happen any more.

vargaz added a commit to vargaz/runtime that referenced this pull request May 1, 2023
@vargaz
Reenable a test which doesn't seem to fail any more.
6be92c2 
Re: dotnet#82663.
vargaz added a commit that referenced this pull request May 1, 2023
@vargaz
Reenable a test which doesn't seem to fail any more. (#85576)
d75e811 
Re: #82663.
@vargaz
Copy link
Contributor Author

vargaz commented May 1, 2023

#85606

@ghost ghost locked as resolved and limited conversation to collaborators May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@lewing lewing lewing approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees

@vargaz vargaz

Labels
area-Codegen-JIT-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mono: Crash when accessing a field of a null class object
6 participants
@vargaz @lambdageek @mangod9 @lewing @MichalStrehovsky @marek-safar