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

[wasm?] Mono interpreter vector argument alignment issue #85071

Closed
kg opened this issue Apr 19, 2023 · 9 comments
Closed

[wasm?] Mono interpreter vector argument alignment issue #85071

kg opened this issue Apr 19, 2023 · 9 comments
Assignees
Labels
Milestone

Comments

@kg
Copy link
Member

kg commented Apr 19, 2023

This may be a dupe of some existing issues, and is probably what's blocking the interp PackedSimd PR.

A reduced repro is to run the SIMD version of pavel's raytracer in the browser with interp simd turned on. Scene creation fails, and with logging added we can see that the arguments to the ctor seem to be misaligned (the args are vectors with references and scalars mixed in):

calling InfinitePlane(position=0,1,0,0, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 2f), normalDirection=Util.UpVector(0f, 1f, 0f, 0f), cellWidth=10f
InfinitePlane(position=<0, 0, 0, 1>, material=, normalDirection=<1, 1, 0, 1>, cellWidth=0
calling InfinitePlane(position=1,2,3,4, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 12f), normalDirection=-Util.UpVector(0f, -1f, 0f, 0f), cellWidth=7f
dotnet.wasm:0x1700f Uncaught RuntimeError: memory access out of bounds
    at dotnet.wasm:0x1700f

Based on this it looks like the vector elements are packed with extra space between them, and one of the zeroes from the first position vector crowds into the material slot, turning it into a null. The second position has no zeroes so it becomes a garbage pointer and we get a crash.

How to run:

dotnet.sh publish -c Release /bl /p:TargetOS=browser /p:TargetArchitecture=wasm RayTracer.csproj /p:EnableAOTAndTrimming=false
dotnet.sh serve --directory bin/Release/AppBundle/

dotnet-wasm-raytracer-simd.zip

Since interp simd is disabled for wasm right now, you'll want to use the current HEAD of my SIMD PR #82773, which is configured for testing (a bunch of stuff is disabled).

@kg kg added arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono labels Apr 19, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 19, 2023
@ghost
Copy link

ghost commented Apr 19, 2023

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

Issue Details

This may be a dupe of some existing issues, and is probably what's blocking the interp PackedSimd PR.

A reduced repro is to run the SIMD version of pavel's raytracer in the browser with interp simd turned on. Scene creation fails, and with logging added we can see that the arguments to the ctor seem to be misaligned (the args are vectors with references and scalars mixed in):

calling InfinitePlane(position=0,1,0,0, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 2f), normalDirection=Util.UpVector(0f, 1f, 0f, 0f), cellWidth=10f
InfinitePlane(position=<0, 0, 0, 1>, material=, normalDirection=<1, 1, 0, 1>, cellWidth=0
calling InfinitePlane(position=1,2,3,4, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 12f), normalDirection=-Util.UpVector(0f, -1f, 0f, 0f), cellWidth=7f
dotnet.wasm:0x1700f Uncaught RuntimeError: memory access out of bounds
    at dotnet.wasm:0x1700f

Based on this it looks like the vector elements are packed with extra space between them, and one of the zeroes from the first position vector crowds into the material slot, turning it into a null. The second position has no zeroes so it becomes a garbage pointer and we get a crash.

How to run:

dotnet.sh publish -c Release /bl /p:TargetOS=browser /p:TargetArchitecture=wasm RayTracer.csproj /p:EnableAOTAndTrimming=false
dotnet.sh serve --directory bin/Release/AppBundle/

dotnet-wasm-raytracer-simd.zip

Since interp simd is disabled for wasm right now, you'll want to use the current HEAD of my SIMD PR #82773, which is configured for testing (a bunch of stuff is disabled).

Author: kg
Assignees: -
Labels:

arch-wasm, area-Codegen-Interpreter-mono

Milestone: -

@kg
Copy link
Member Author

kg commented Apr 19, 2023

This is also broken on current main.

@kg
Copy link
Member Author

kg commented Apr 19, 2023

Modifying the Scene ctor to use temporary locals for the vectors changes the (still incorrect) output:

                var v1 = Vector128.Create(0f, 1f, 0f, 0f);
                var m1 = new CheckerboardMaterial(Color.White, Color.Grey, 1.0f, .1f, 0.0f, 2f);
                var v2 = Util.UpVector;
                var f = 10f;
                Console.WriteLine($"v1=={v1}, m1=={m1}, v2=={v2}, f={f}");
                Console.WriteLine($"calling InfinitePlane(position=0,1,0,0, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 2f), normalDirection=Util.UpVector(0f, 1f, 0f, 0f), cellWidth=10f");
                scene.DrawableObjects.Add(new InfinitePlane(v1, m1, v2, f));
v1==<0, 1, 0, 0>, m1==RayTracer.Materials.CheckerboardMaterial, v2==<0, 1, 0, 0>, f=10
dotnet.js:11 calling InfinitePlane(position=0,1,0,0, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 2f), normalDirection=Util.UpVector(0f, 1f, 0f, 0f), cellWidth=10f
dotnet.js:11 InfinitePlane(position=<10, 0, 0, 1>, material=, normalDirection=<1E-45, 0, 0, 1>, cellWidth=0
dotnet.js:11 calling InfinitePlane(position=1,2,3,4, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 12f), normalDirection=-Util.UpVector(0f, -1f, 0f, 0f), cellWidth=7f
dotnet.wasm:0x16c52 Uncaught RuntimeError: memory access out of bounds

The local vectors appear to not be corrupt, and tostringing them in the format string is working right. But then they are corrupt in a different way when passed through to the ctor.

@SamMonoRT SamMonoRT added this to the 8.0.0 milestone Apr 25, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 25, 2023
@SamMonoRT
Copy link
Member

@BrzVlad - likely fixed by your open PR ?

@kg
Copy link
Member Author

kg commented Apr 27, 2023

If vlad doesn't have time, once I have access to my dev workstation again I will re-test this on latest main.

@kg
Copy link
Member Author

kg commented Apr 30, 2023

Running this test against #85153 , it still fails

@kg
Copy link
Member Author

kg commented May 4, 2023

The issue (as far as I can tell) is that the arguments in the constructor are misaligned by 8 bytes (one stackval), maybe the size of 'this' is being calculated incorrectly as part of the call?

From my updated test:

calling InfinitePlane(position=0,1,0,0, material=CheckerboardMaterial(White, Grey, 1f, .1f, 0f, 2f), normalDirection=Util.UpVector(0f, 1f, 0f, 0f), cellWidth=10f

&position=44630464, &material=44630480, &normalDirection=44630496, &cellWidth=44630512
InfinitePlane(position=<2.4833143E-37, 0, 0, 1>, material=, normalDirection=<1E-45, 0, 0, 1>, cellWidth=0
position+8=<0, 1, 0, 0>, material+8=RayTracer.Materials.CheckerboardMaterial, normalDirection+8=<0, 1, 0, 0>

The values of the arguments are incorrect, but if I use Unsafe.AddByteOffset to add 8 bytes to them, I am able to read all the arguments no problem. So the interp is passing all the arguments but the callee is confused about their location.

@kg
Copy link
Member Author

kg commented May 4, 2023

Vlad helped me investigate this some. The problem seems to be specific to MINT_NEWOBJ_SLOW_UNOPT, which only gets used for methods that aren't tiered - so if a method has been optimized or tiering is disabled, the issue goes away.
The problem seems to be that the call logic and the generated code for the ctor disagree on whether the arguments on the stack are SIMD-aligned. If I disable this:

if (ip [6]) // Check if first arg is simd type, which requires realigning param area
param_offset = ALIGN_TO (call_args_offset + MINT_STACK_SLOT_SIZE, MINT_SIMD_ALIGNMENT);

Then the ctors in the test case are able to find their arguments properly, but the raytracer crashes later in startup. I tried various potential changes to try and align the arguments but none of them worked, only this.
The problem appears to affect (among others) both the Camera and InfinitePlane ctors in the sample, since both specifically have a simd type as their first argument. It seems like the bug may be limited to that, 'ctor with a vector as arg1'.

@BrzVlad
Copy link
Member

BrzVlad commented May 8, 2023

Fixed by #85787

@BrzVlad BrzVlad closed this as completed May 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants