Fix and Test case for #27924 #1059

CarolEidt · 2019-12-19T17:01:28Z

https://github.com/dotnet/coreclr/issues/27924

…perand, to preserve GC-ness.

mikedn · 2019-12-19T22:26:45Z

src/coreclr/src/jit/codegenxarch.cpp

@@ -954,7 +954,7 @@ void CodeGen::genCodeForBinary(GenTreeOp* treeNode)
    // reg3 = reg3 op reg2
    else
    {
-        inst_RV_RV(ins_Copy(targetType), targetReg, op1reg, targetType);
+        inst_RV_RV(ins_Copy(targetType), targetReg, op1reg, op1->TypeGet());


Hmm, should gcMarkRegPtrVal below also use op1->TypeGet()?

Interesting - it probably should. It gets set to a byref during emit, which fixes the GC info. I confess that I'm not sure how regSet.m_rsGCInfo.gcRegByrefSetCur is used during the codegen phase, but I presume that, since it's maintained, it must be depended on. I'll go ahead and change that and dig a little deeper in the meantime.

AFAICT, the codegen-time gc info is used at labels and calls, and hence, since this is a transitory byref, it wouldn't show up as being wrong in this case. That said, I believe it should be maintained correctly.

CarolEidt · 2019-12-20T15:55:00Z

@dotnet/dnceng - I am getting package failures even on retry:

root/runtime/src/installer/pkg/projects/netcoreapp/sfx/Microsoft.NETCore.App.SharedFx.sfxproj(0,0): error NU1102: Unable to find package Microsoft.NETCore.Platforms with version (>= 5.0.0-ci.19620.1)

Found 1 version(s) in /root/runtime/artifacts/packages/Release/Shipping/ [ Nearest version: 5.0.0-ci.19619.1 ]
Found 0 version(s) in /root/runtime/artifacts/packages/Release/NonShipping/

CarolEidt · 2019-12-20T15:57:51Z

PTAL @dotnet/jit-contrib
cc @jkotas
The test failed in the second commit. The 3rd and 4th commits have the fix.
Once I get a clean CI run I'll change the priority of the test to 1. It has GC stress set so it takes a few seconds to run.

mmitche · 2019-12-20T16:22:34Z

@dotnet/dnceng - I am getting package failures even on retry:

root/runtime/src/installer/pkg/projects/netcoreapp/sfx/Microsoft.NETCore.App.SharedFx.sfxproj(0,0): error NU1102: Unable to find package Microsoft.NETCore.Platforms with version (>= 5.0.0-ci.19620.1)

Found 1 version(s) in /root/runtime/artifacts/packages/Release/Shipping/ [ Nearest version: 5.0.0-ci.19619.1 ]

Found 0 version(s) in /root/runtime/artifacts/packages/Release/NonShipping/

That looks like a build issue. @ViktorHofer @dagood I thought the live-live build was working now? That looks like it's referencing packages from yesterday?

jkotas · 2019-12-20T16:29:56Z

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27924/GitHub_27924.cs

+
+    static void Work()
+    {
+        for (uint i = 0; i < 1000000; i++) {


Nit: { placement is inconsistent

jkotas

Thanks!

dagood · 2019-12-20T16:32:00Z

That looks like a build issue. @ViktorHofer @dagood I thought the live-live build was working now? That looks like it's referencing packages from yesterday?

#839 documents that you need to do a fresh build whenever you cross a day boundary. Looks like it applies to CI as well.

I believe this is a combination of:

OfficialBuildId isn't passed in, which makes the version number non-repeatable.
We use date-based versions in our non-official builds in the first place. I dislike this, and filed Can we use "dev" prerelease tag for local builds, rather than "dev.12345.1"? #246 a while back for it.

dagood · 2019-12-20T16:41:26Z

It looks like the original error was also caused by UTC day tickover. Here's the timestamp on the first line of a failing Installer build step in attempt 1:

2019-12-20T00:03:04.6739571Z

Core-Setup stopped having this problem once it moved off BuildTools onto Arcade. Now we've got it again because of global dotnet/runtime settings.

Right now, any build where Libraries => Installer spans a UTC day should fail this way. Adding it to the CI problem tracking issue #702.

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27924/GitHub_27924.csproj

jashook

Approved but meant to comment.

Having an innerloop test which sets GCStress 0xC seems incorrect. We know that gc stress has a certain level of unreliability that does not fit with our innerloop test bar.`

CarolEidt · 2019-12-20T21:27:30Z

@dotnet/dnceng - I re-pushed (with a change to address PR feedback), but I'm still getting some inscrutable failures - e.g. https://helix.dot.net/api/2019-06-17/jobs/fda888e8-b6d4-40fc-ba3f-7fb296c47338/workitems/JIT.jit64.mcc/console simply ends with:

2019-12-20T15:52:00.450Z ERROR xunit-reporter.py xunit-reporter(80) main Unable to report xunit results: no test results xml file found.

Any suggestions for how to move this forward?

dagood · 2019-12-20T21:35:56Z

I think that's #1097, @trylek is going to look at that. The log line to focus on seems to be set _commandExitCode=-1073741701, (‭C000007B‬, STATUS_INVALID_IMAGE_FORMAT). It's happening on other PRs too.

BruceForstall · 2020-01-06T17:42:36Z

@CarolEidt The remaining CI failures are known Windows arm failures, so is this ready to merge?

CarolEidt · 2020-01-07T16:19:45Z

is this ready to merge?

Since this adds a new test, I was hoping to get arm testing on this, but perhaps at this point I should just build and test on arm myself.

This is the fix for #27924. This is a GC hole bug that was found externally, #27590. The cause is that the JIT was using the target type of the subtract when it needed to make a copy of the source, but it needs to use the source type. ## Customer Impact Corruption of state that is non-deterministic and hard to track down. ## Regression? Not a recent regression, but exposed by Unsafe.ByteOffset. ## Testing The fix has been verified in the runtime repo. ## Risk Low: The fix is straightfoward and only impacts 3 lines of code.

CarolEidt added 2 commits December 19, 2019 09:00

Test case for dotnet#27924

7e8c9e9

Temporarily make this a pri0 test to get ci coverage

31d0c00

jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 19, 2019

Use the source type when copying a source to the target of a binary o…

379bc01

…perand, to preserve GC-ness.

mikedn reviewed Dec 19, 2019

View reviewed changes

Fix the codegen-time gcInfo

4ff9848

CarolEidt changed the title ~~Test case for #27924~~ Fix and Test case for #27924 Dec 20, 2019

jkotas reviewed Dec 20, 2019

View reviewed changes

jkotas approved these changes Dec 20, 2019

View reviewed changes

jashook mentioned this pull request Dec 20, 2019

Infrastructure - Status/Health #702

Closed

dagood mentioned this pull request Dec 20, 2019

When PR validation spans a UTC day, the Installer build may fail due to a package version mismatch #1089

Closed

Clean up test case

7c1a25c

jashook approved these changes Dec 20, 2019

View reviewed changes

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_27924/GitHub_27924.csproj Show resolved Hide resolved

jashook reviewed Dec 20, 2019

View reviewed changes

jashook approved these changes Dec 20, 2019

View reviewed changes

CarolEidt closed this Dec 26, 2019

CarolEidt reopened this Dec 26, 2019

CarolEidt merged commit 685406a into dotnet:master Jan 10, 2020

CarolEidt deleted the Fix27924 branch January 10, 2020 17:33

Anipik mentioned this pull request Jul 17, 2020

Span CopyTo Span - Crashes .Net #13701

Closed

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and Test case for #27924 #1059

Fix and Test case for #27924 #1059

CarolEidt commented Dec 19, 2019 •

edited by sandreenko

Loading

mikedn Dec 19, 2019

CarolEidt Dec 19, 2019

CarolEidt Dec 19, 2019

CarolEidt commented Dec 20, 2019

CarolEidt commented Dec 20, 2019

mmitche commented Dec 20, 2019

jkotas Dec 20, 2019

jkotas left a comment

dagood commented Dec 20, 2019

dagood commented Dec 20, 2019

jashook left a comment

CarolEidt commented Dec 20, 2019

dagood commented Dec 20, 2019 •

edited

Loading

BruceForstall commented Jan 6, 2020

CarolEidt commented Jan 7, 2020

Fix and Test case for #27924 #1059

Fix and Test case for #27924 #1059

Conversation

CarolEidt commented Dec 19, 2019 • edited by sandreenko Loading

mikedn Dec 19, 2019

Choose a reason for hiding this comment

CarolEidt Dec 19, 2019

Choose a reason for hiding this comment

CarolEidt Dec 19, 2019

Choose a reason for hiding this comment

CarolEidt commented Dec 20, 2019

CarolEidt commented Dec 20, 2019

mmitche commented Dec 20, 2019

jkotas Dec 20, 2019

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

dagood commented Dec 20, 2019

dagood commented Dec 20, 2019

jashook left a comment

Choose a reason for hiding this comment

CarolEidt commented Dec 20, 2019

dagood commented Dec 20, 2019 • edited Loading

BruceForstall commented Jan 6, 2020

CarolEidt commented Jan 7, 2020

CarolEidt commented Dec 19, 2019 •

edited by sandreenko

Loading

dagood commented Dec 20, 2019 •

edited

Loading