[mini] remove assert in mono_local_regalloc

[Rosalie241]

Fixes Pop'n Music Be-Mouse crashing on that assert.

[madewokherd]

Is this going to randomly break things if the code generator changes or a different method hits that assert?

[Rosalie241]

well, the game doesn't crash at all with this patch applied, as for other programs, I personally doubt it'd break things because if an application hits that assert, it'll crash technically anyways.

[Rosalie241]

So unless an application already hits that assert (unlikely), it wont be affected.

[madewokherd]

My concern is that if it generates incorrect code, it could break in more subtle ways that are harder to diagnose.

[Rosalie241]

well, a more proper solution would be to have a proper implementation instead of a FIXME & assert there, but I'm not familiar enough with mono's giant codebase to delve into that.

[madewokherd]

I'll take a look when I'm back at work. If it's too hard, we can have it print a warning.

[madewokherd]

OK, I've never worked with this code, and there is a lot here. My best guess right now is: mini is compiling an instruction where one of the inputs is in a register that has been spilled into the stack, and it needs to be copied to a specific hardware register required by the instruction. So we need it to generate an indirect read instruction to copy to the required hardware register. I think the instruction needs to be selected from regbank_load_ops, but unlike the existing uses we need to do it using the vassign value to select the spill variable.

That seems like it shouldn't be too difficult, so I'd like to try that and send it upstream if possible.

I'm guessing there's no practical way I could legally obtain this software.

Would you be willing to get some debug output so I can verify my understanding and maybe write a test case? You should be able to figure out which method is causing this error by running with WINE_MONO_VERBOSE=1. You can then set MONO_VERBOSE_METHOD to the method name to get debug info from the JIT for that method.

[Rosalie241]

Sadly the moment I launch it with WINE_MONO_VERBOSE=1, it crashes on startup, while if I launch it without that environment variable, it doesn't crash:

output of WINE_MONO_VERBOSE=1 wine popnmusicBeMouse.exe: output.txt

[Rosalie241]

I should also mention this executable is a bit odd, it seems to contain IL and C++ (possibly C++/CLI?), it's originally made for .NET 1.

[madewokherd]

Ah right, there's a bug where wine's debug code aborts if an output line is too long.

[Rosalie241]

I managed to increase the WINE debug output string size (and recompile WINE), this is the output of
MONO_VERBOSE_METHOD="<Module>:PerformanceCounter.Stop" wine popnmusicBeMouse.exe: mono-debug.log

PerformanceCounter.Stop is the last line in the output of WINE_MONO_VERBOSE=1 before it reaches the assert:

Method (wrapper native-to-managed) void* modopt(System.Runtime.CompilerServices.CallConvThiscall) <Module>:DirectX.Direct3D.D3DRect.__vecDelDtor (DirectX.Direct3D.D3DRect* modopt(Microsoft.VisualC.IsConstModifier) modopt(Microsoft.VisualC.IsConstModifier),uint) emitted at 03173650 to 031736dc (code length 140) [popnmusicBeMouse.exe]
converting method void* modopt(System.Runtime.CompilerServices.CallConvThiscall) <Module>:DirectX.Direct3D.D3DRect.__vecDelDtor (DirectX.Direct3D.D3DRect* modopt(Microsoft.VisualC.IsConstModifier) modopt(Microsoft.VisualC.IsConstModifier),uint)
Method void* modopt(System.Runtime.CompilerServices.CallConvThiscall) <Module>:DirectX.Direct3D.D3DRect.__vecDelDtor (DirectX.Direct3D.D3DRect* modopt(Microsoft.VisualC.IsConstModifier) modopt(Microsoft.VisualC.IsConstModifier),uint) emitted at 0d33f9d8 to 0d33fa6d (code length 149) [popnmusicBeMouse.exe]
converting method long modopt(System.Runtime.CompilerServices.CallConvThiscall) <Module>:PerformanceCounter.Stop (PerformanceCounter* modopt(Microsoft.VisualC.IsConstModifier) modopt(Microsoft.VisualC.IsConstModifier),int)
* Assertion: should not be reached at /vagrant/mono/mono/mini/mini-codegen.c:1460

[madewokherd]

Thanks. Unfortunately, I think the part that's failing is calculating a pointer to receive the result of QueryPerformanceCounter, so this is likely to break if it's not fixed properly.

I'm confused because, from what I can tell, there's no particular requirement that the inputs to the failing instruction (int_shl R44 <- R42 R93) be in any particular register, they just need to be in a hardware register. So I'm a little confused about how to decide which hardware register to use. Otherwise, I think loading by generating an indirect read instruction, as (int_shl R57 <- R55 R93) already does, is the right thing to do. I'll have to see if I can understand how that other case works.

[madewokherd]

I think this is too complicated for me to write a testcase, but I wrote a patch. Could you let me know if this fixes it, and if it does get another MONO_VERBOSE_METHOD log so I can see if the code it generates looks right?
codegen.txt

[Rosalie241]

that patch seems to work fine, here's the debug output with the patch: mono-debug2.log

[madewokherd]

Yay, and the output looks right to me. I'll try sending that upstream.

[madewokherd]

I went ahead and pushed the fix because sadly I've gotten used to upstream failing to review things, and I want to make a release soon. b8d4d95

I'll close this when the fix lands in a release.

[Rosalie241]

Thank you very much ❤️

[madewokherd]

For completeness, here's a CI build with the fix: https://github.com/madewokherd/wine-mono/actions/runs/1045717236

[madewokherd]

The fix was included in https://github.com/madewokherd/wine-mono/releases/tag/wine-mono-6.3.0 which will be in Wine 6.14. Closing this.