Fix GCStress failures from hardware intrinsic tests by fiigii · Pull Request #17314 · dotnet/coreclr

fiigii · 2018-03-29T08:29:41Z

fix https://github.com/dotnet/coreclr/issues/17278

mikedn · 2018-03-29T08:46:39Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/AlternatingBinOpTest_DataTable.cs


-            Unsafe.CopyBlockUnaligned(ref Unsafe.AsRef<byte>(inArray1Ptr), ref Unsafe.As<TOp1, byte>(ref inArray1[0]), this.simdSize);
-            Unsafe.CopyBlockUnaligned(ref Unsafe.AsRef<byte>(inArray2Ptr), ref Unsafe.As<TOp2, byte>(ref inArray2[0]), this.simdSize);
+            Unsafe.CopyBlockUnaligned(ref Unsafe.AsRef<byte>(inArray1Ptr), ref Unsafe.As<TOp1, byte>(ref inArray1[0]), inArray1.Length * sizeof(TOp1));


It's not obvious to me why the old code was bad and the new code is correct, the array length is simdSize * 2 so it seems that copying simdSize bytes should have worked correctly. Besides, I doubt that sizeof(TOp1) will compile.

The problem was the size of arguments (inArray1, inArray2, outArray, etc) could be different in AVX/AVX2 intrinsics.

the array length is simdSize * 2

simdSize * 2 is the length of buffers (the members) rather than input arrays (the arguments).

Besides, I doubt that sizeof(TOp1) will compile.

I tried, it works well.

The code in this file is plain bogus...

simdSize * 2 is the length of buffers (the members) rather than input arrays (the arguments).

Right. And that is odd since the buffers end up being smaller than the input arrays. I assume that simdSize is the size of a vector.

Oops, sizeof(TOp1) does not work...

Actually, it is the largest alignment boundary.

Isn't that the same thing as the vector size? SSE2 vector size is 16 bytes and the required alignment is 16 bytes. CopyBlockUnaligned copies bytes the old code copied 16 bytes. How could the input arrays be smaller than 16 bytes?

For any given test, simdSize should be the size of the largest Vector used (so 16 or 32, currently).

We then allocate 2 * simdSize so that we can guarantee the buffer is aligned (since you need at most alignment - 1 additional bytes to guarantee alignment).

In the case where one argument is Vector128 and one argument is Vector256, simdSize will be larger than the input array for the Vector128 args, and will result in an AV.

Except you don't usually get an AV. The code just keeps copying whatever random bytes there are past the end of the array. So the failure only shows up in GC stress when we break in just after the copy and find there is a byref and it happens to be pointing into free space.

In the case where one argument is Vector128 and one argument is Vector256, simdSize will be larger than the input array for the Vector128 args, and will result in an AV.

I see. Well, this code is a mess.

The DataTable name is misleading, this whole class isn't some sort of data table, it's just some kind of wrapper for some aligned memory buffers (and it's not clear why instead of simply having a AlignedArray<T> class we have instead this set of classes where the logic is duplicated).

The simdSize parameter should have been called alignment, that's its main purpose here.

There are no asserts that would suggest that the input arrays are supposed to contain only one vector and thus be smaller than simdSize.

In the absence of such asserts the length of the new arrays should have been calculated based on the parameter arrays, not based on simdSize.

The out array parameter is not used

The way alignment is done is sort of unusual. Pretty much every alignment function I've seen does (x + (alignment - 1)) & ~(alignment - 1).

It probably would have been simpler to just allocate memory using Marshal.AllocCoTaskMem instead of dragging in GC handles as well

The actual test code isn't any better. Tons of repeated code, unsafe copies and whatnot. I take it that people were in a hurry to get things done and this is the result.

fiigii · 2018-03-29T10:03:41Z

test Windows_NT x86 Checked gcstress0xc_jitstress1
test Windows_NT x86 Checked gcstress0xc_jitstress2

fiigii · 2018-03-29T15:55:23Z

@AndyAyersMS @CarolEidt @tannergooding

tannergooding · 2018-03-29T16:02:19Z

@fiigii, we fixed this in the other templates by doing (uint)Unsafe.SizeOf<{Op1VectorType}<{Op1BaseType}>>() (in the template).

We should probably do the same here.

Edit: Actually, we don't have the Op1VectorType here, so this is probably the closest we will get...
Maybe it is worth asserting that the number of bytes copied is less than or equal to the size of the allocated block?

AndyAyersMS · 2018-03-29T16:13:15Z

With these changes I can see that the copy won't read of the end of the parameter inArray but what guarantee is there now that the copy won't write past the end of the local inArray?

Seems like we are relying on the callers to pass reasonable values for parameters and I suspect as people try and add tests they will get this wrong sometimes.

A few lines of validation checking can catch these issues without having to debug through a gc stress failure. So I suggest adding some here.

fiigii · 2018-03-29T19:03:35Z

Addressed feedback. @AndyAyersMS @mikedn @tannergooding PTAL

AndyAyersMS · 2018-03-29T19:21:38Z

We only create r and ro build variants for these tests so we'll never see these asserts fire.

Please use explicit throws to ensure that validation is always done no matter how the test is built.

fiigii · 2018-03-29T19:41:37Z

@AndyAyersMS Thanks for the suggestion. Fixed.

AndyAyersMS

LGTM save for the one typo.

AndyAyersMS · 2018-03-29T19:51:27Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/AlternatingBinOpTest_DataTable.cs

+            int sizeOfoutArray = outArray.Length * Unsafe.SizeOf<TResult>();
+            if ((alignment != 32 && alignment != 16) || (alignment * 2) < sizeOfinArray1 || (alignment * 2) < sizeOfinArray2 || (alignment * 2) < sizeOfoutArray)
+            {
+                throw new ArgumentException("Invalid vlue of alignment");


Nit: "vlue" --> "value" (copied thoughout)

Oops, will fix!

AndyAyersMS · 2018-03-29T23:23:40Z

Hmm, think jenkins lost it there for a minute. Let me close/repoen.

fiigii · 2018-03-30T00:18:39Z

test Windows_NT x86 Checked gcstress0xc_jitstress1
test Windows_NT x86 Checked gcstress0xc_jitstress2
test Windows_NT x64 Checked jitx86hwintrinsicnoavx
test Windows_NT x86 Checked jitx86hwintrinsicnoavx
test Ubuntu x64 Checked jitx86hwintrinsicnoavx

AndyAyersMS · 2018-03-30T07:04:43Z

The "hit priveleged instruction" failure in x86 gc stress 0xC jitstress 1 is a symptom of a gc stress race that I'm trying to fix over in #17330.

The "SanityCheck()" failure in x86 gc stress 0xC jitstress 2 could be a number of things but it's unrelated to this change.

fiigii · 2018-03-30T16:53:12Z

Can we merge this PR?

CarolEidt

LGTM

fiigii force-pushed the fixgctests branch from 22ad206 to 0027161 Compare March 29, 2018 08:34

mikedn reviewed Mar 29, 2018

View reviewed changes

fiigii force-pushed the fixgctests branch from 0027161 to 38338be Compare March 29, 2018 10:03

fiigii force-pushed the fixgctests branch from 38338be to 2e67f3c Compare March 29, 2018 19:00

fiigii mentioned this pull request Mar 29, 2018

Fix the code-size estimation for AVX/AVX2 ExtractVector128 #17180

Merged

fiigii force-pushed the fixgctests branch from 2e67f3c to c90cc9a Compare March 29, 2018 19:39

AndyAyersMS reviewed Mar 29, 2018

View reviewed changes

fiigii closed this Mar 29, 2018

fiigii force-pushed the fixgctests branch from c90cc9a to e8eb5c6 Compare March 29, 2018 22:38

Fix GCStress failures

10f51a0

fiigii reopened this Mar 29, 2018

AndyAyersMS closed this Mar 29, 2018

AndyAyersMS reopened this Mar 29, 2018

CarolEidt approved these changes Mar 30, 2018

View reviewed changes

CarolEidt merged commit 17de92e into dotnet:master Mar 30, 2018

fiigii deleted the fixgctests branch August 29, 2018 00:01

Conversation

fiigii commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii commented Mar 29, 2018

Uh oh!

fiigii commented Mar 29, 2018

Uh oh!

tannergooding commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Mar 29, 2018

Uh oh!

fiigii commented Mar 29, 2018

Uh oh!

AndyAyersMS commented Mar 29, 2018

Uh oh!

fiigii commented Mar 29, 2018

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Mar 29, 2018

Uh oh!

fiigii commented Mar 30, 2018

Uh oh!

AndyAyersMS commented Mar 30, 2018

Uh oh!

fiigii commented Mar 30, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fiigii Mar 29, 2018 •

edited

Loading

tannergooding commented Mar 29, 2018 •

edited

Loading