Re: [PATCH] mscoree: Implement VTable fixup for x86_64 architecture.
I don't see anything obviously wrong here, but I also have no way of testing it, since we can't compile methods without loading the class library dll's.
+#elif __x86_64__ /* !defined(__i386__) */
Later in the patch it says defined(__x86_64__). I have no idea whether this distinction matters.
On 01/20/2016 11:07 PM, Vincent Povirk wrote:
I don't see anything obviously wrong here, but I also have no way of testing it, since we can't compile methods without loading the class library dll's. I have progressed a bit in attempts to load mixed code DLL into 64-bit process. To do this I built wine-mono pure assembly dlls (mscorlib and others) with -platform:x64 flag which provides x86_64 pure assembly dlls and thus allows to workaround DLL load issue.
It crashes from within erroneously called callbacks on mono init. This is fixed by my 2nd patch. It helps to pass through init and to see messages from mono which are displayed through a callback. In this state ReallyFixupVTable is called from x86_64 thunk, where mono_marshal_get_vtfixup_ftnptr() starts executing 64-bit mixed code DLL's assembly initialization, but currently fails with my DLL on "invalid IL instruction" somewhere around System.IntPtr.ctor call (the same class works on i686), which is probably some mono issue. So I cannot come up with a fully working test case for now, though I did not give up yet.
+#elif __x86_64__ /* !defined(__i386__) */ Later in the patch it says defined(__x86_64__). I have no idea whether this distinction matters.
Comments say "defined", while there are #if's. It was originally like this when it was __i386__ only, I tried to minimize the changes. #if does work both for i386 and x86_64 (though #ifdef would work either). Should I fix comments for consistency?
I have progressed a bit in attempts to load mixed code DLL into 64-bit process. To do this I built wine-mono pure assembly dlls (mscorlib and others) with -platform:x64 flag which provides x86_64 pure assembly dlls and thus allows to workaround DLL load issue.
Interesting. Do you have a patch to Mono that does this?
In this state ReallyFixupVTable is called from x86_64 thunk, where mono_marshal_get_vtfixup_ftnptr() starts executing 64-bit mixed code DLL's assembly initialization, but currently fails with my DLL on "invalid IL instruction" somewhere around System.IntPtr.ctor call (the same class works on i686), which is probably some mono issue. So I cannot come up with a fully working test case for now, though I did not give up yet.
I can try to build a 64-bit managed C++ dll in visual studio and call it from a C program. If Mono still has trouble with that, I think there are ways to use vtable fixups in (otherwise) pure managed code to export methods to C.
+#elif __x86_64__ /* !defined(__i386__) */ Later in the patch it says defined(__x86_64__). I have no idea whether this distinction matters.
Comments say "defined", while there are #if's. It was originally like this when it was __i386__ only, I tried to minimize the changes. #if does work both for i386 and x86_64 (though #ifdef would work either). Should I fix comments for consistency?
I guess it's not important then. If you're touching those lines anyway, I guess it makes sense to fix it. But more importantly, I don't think we should commit this until we can test it somehow.
On 01/21/2016 12:13 AM, Vincent Povirk wrote:
I have progressed a bit in attempts to load mixed code DLL into 64-bit process. To do this I built wine-mono pure assembly dlls (mscorlib and others) with -platform:x64 flag which provides x86_64 pure assembly dlls and thus allows to workaround DLL load issue. Interesting. Do you have a patch to Mono that does this?
I just hot fixed it in a makefile after standard configuration & first time build. File is mono/mcs/build/library.make, line 283 (recipe for $(build_lib): $(LIBRARY_COMPILE) $(LIBRARY_FLAGS) $(LIB_MCS_FLAGS) $(MCS_FLAGS_RESOURCE_STRINGS) -target:library -platform:x64 -debug+ -out:$@ $(BUILT_SOURCES_cmdline) @$(response) I inserted -platform:x64 to the original line. I also commented out next line '$(Q) $(SN) -R $@ $(LIBRARY_SNK)' as assembly signing did not work for me for x64 arch dlls. I tested the unsigned DLLs with win32 setup, it worked OK so I suppose this is not a problem for testing. After that 'make clean; make' in mono/mcs/class does the job without rebuilding the whole thing. Then I just copied the dlls to mono dir in wineprefix.
In this state ReallyFixupVTable is called from x86_64 thunk, where mono_marshal_get_vtfixup_ftnptr() starts executing 64-bit mixed code DLL's assembly initialization, but currently fails with my DLL on "invalid IL instruction" somewhere around System.IntPtr.ctor call (the same class works on i686), which is probably some mono issue. So I cannot come up with a fully working test case for now, though I did not give up yet. I can try to build a 64-bit managed C++ dll in visual studio and call it from a C program. I can send a simple test program which I am using now if it helps. It is a bit weird though as using MFC in the main app. And I still have a problem running it in wine64 ('System.InvalidProgramException: Invalid IL code in <Module>:gcroot<System::String ^>.= (gcroot<System::String ^>*,string): IL_0004: call 0x0a000012' on calling IntPtr.ctor(void*)) which I am trying to fix. So maybe a simpler one could help. Unfortunately I don't have MSVC and cannot build mixed assemblies myself.
I guess it's not important then. If you're touching those lines anyway, I guess it makes sense to fix it. But more importantly, I don't think we should commit this until we can test it somehow. Yes, sure
I can send a simple test program which I am using now if it helps. It is a bit weird though as using MFC in the main app. And I still have a problem running it in wine64 ('System.InvalidProgramException: Invalid IL code in <Module>:gcroot<System::String ^>.= (gcroot<System::String ^>*,string): IL_0004: call 0x0a000012' on calling IntPtr.ctor(void*)) which I am trying to fix. So maybe a simpler one could help. Unfortunately I don't have MSVC and cannot build mixed assemblies myself.
If you have Windows and would like to try, you may be able to use Community Edition for this.
Hi Vincent, I managed to run x86_64 mixed assembly and to test that my x64 VTable fixup is actually working so far. This required fixing mono assembly compiler. It has a few bugs on checking assembly in 'check_call_signature' and in 'type_from_op' (it sometimes forgets that ptrs are 8 bytes in x64). I am attaching a patch for wine-mono which includes the fixes I made to compiler and also the "quick-fix" changes to makefiles (which are really dumb). The patch for assembly compiler is not complete. There are other num tables which are incorrect for x64 also but I did not fall to an error with them in my test case so I did not touch them. I guess the assembly compiler issue should affect not only our case but mono64 also. Now I will get back to my real app, but most likely I will fall into the problem that in real app there are pure .Net i386 dlls which are used from mixed x86_64. Do you want me to send my test case (MSVC source code and/or compiled), or do something else to make my test reproducible? Thanks, Paul.
Changes to Mono should ideally be sent upstream. For code generation issues you could add a .il test in mono/tests. This case is sort of complicated because it sounds like the code's validity depends on the architecture. (But that's also worth testing on .NET I guess.) If we can't get the patch upstream, and it helps Windows apps, then I can maintain a diff in wine-mono.
Frankly I did not intend fixing mono compiler and modifying mono build environment in a way that would look like a good patch to send. I presume it is rather long way to go to get it done and accepted, while I do not deal with .NET/mono in my everyday life. Maybe I could submit a bug report there instead attaching my patch as an illustration of what is happening and what sort of Mono libs build could be helpful for 64bit Mono in Wine? Do you think there is any way we can test the current (probably much simpler) VTable issue? I could send, for instance, a prebuilt MSI with these things hotfixed. BTW my real application (its .net parts) worked after all these fixes. There is still some crash both in my app and short test on exit from application on mono assembly deinit, but this is likely a quite separate issue and happens in 32 bit also. On 01/21/2016 10:14 PM, Vincent Povirk wrote:
Changes to Mono should ideally be sent upstream. For code generation issues you could add a .il test in mono/tests. This case is sort of complicated because it sounds like the code's validity depends on the architecture. (But that's also worth testing on .NET I guess.)
If we can't get the patch upstream, and it helps Windows apps, then I can maintain a diff in wine-mono.
Frankly I did not intend fixing mono compiler and modifying mono build environment in a way that would look like a good patch to send. I presume it is rather long way to go to get it done and accepted, while I do not deal with .NET/mono in my everyday life.
For loading class libraries, the right solution is to fix Wine so that it can load pure .NET images in non-x86 processes, using the _CorValidateImage hook. That shouldn't require changes to Mono. It sounded like you had also found a bug in the Mono code generator which could be useful to fix upstream. If you don't want to take the time to get the fix in, maybe you could file a bug with the Mono project containing an example of IL code that triggers it?
Do you think there is any way we can test the current (probably much simpler) VTable issue? I could send, for instance, a prebuilt MSI with these things hotfixed.
I think I have sufficient information now to test this, but I need some time try these things and hopefully catch up with what you've done.
BTW my real application (its .net parts) worked after all these fixes. There is still some crash both in my app and short test on exit from application on mono assembly deinit, but this is likely a quite separate issue and happens in 32 bit also.
Cool. If you have a testcase with source code that triggers the crash on 32-bit, I can look into it. Would you be willing to file a bug for this?
On 01/21/2016 11:05 PM, Vincent Povirk wrote:
It sounded like you had also found a bug in the Mono code generator which could be useful to fix upstream. If you don't want to take the time to get the fix in, maybe you could file a bug with the Mono project containing an example of IL code that triggers it? Will do.
Cool. If you have a testcase with source code that triggers the crash on 32-bit, I can look into it. Would you be willing to file a bug for this? Will do tomorrow. I will try to look a bit more into it first.
I took a different approach to testing this, and it doesn't seem to be working quite right. Instead of building x64 class dll's, I hacked something together based on wine-staging's related patches, mono's implementation of _CorValidateImage, and disabling a check in wineserver. I'm attaching a diff of that. Instead of working around managed c++ issues, I looked for a way to use dll fixups more directly from a C# library: https://sites.google.com/site/robertgiesecke/Home/uploads/unmanagedexports So I have a C# dll that exports an "add" function, like the example, and a win32 console exe that calls it via LoadLibrary/GetProcAddress. On Windows, I can call it multiple times and then exit with no problems. In Wine with your patch, everything works except that the result is incorrect the first time add() is called. I suspect ReallyFixupVTable is changing some registers, and the thunk will have to preserve them. Thanks for working on this, by the way. Seeing this *almost* work even for a simple test case is very cool. :)
Thank you very much. I will check what goes wrong there and fix the patch tomorrow. On Fri, 22 Jan 2016 at 01:33, Vincent Povirk <madewokherd(a)gmail.com> wrote:
I took a different approach to testing this, and it doesn't seem to be working quite right.
Instead of building x64 class dll's, I hacked something together based on wine-staging's related patches, mono's implementation of _CorValidateImage, and disabling a check in wineserver. I'm attaching a diff of that.
Instead of working around managed c++ issues, I looked for a way to use dll fixups more directly from a C# library: https://sites.google.com/site/robertgiesecke/Home/uploads/unmanagedexports
So I have a C# dll that exports an "add" function, like the example, and a win32 console exe that calls it via LoadLibrary/GetProcAddress. On Windows, I can call it multiple times and then exit with no problems. In Wine with your patch, everything works except that the result is incorrect the first time add() is called. I suspect ReallyFixupVTable is changing some registers, and the thunk will have to preserve them.
Thanks for working on this, by the way. Seeing this *almost* work even for a simple test case is very cool. :)
May I ask you to send me the binaries since you already got them? I do not have msvc in place to compile this (at least the same way as you did which might be important). On Fri, 22 Jan 2016 at 01:33, Vincent Povirk <madewokherd(a)gmail.com> wrote:
I took a different approach to testing this, and it doesn't seem to be working quite right.
Instead of building x64 class dll's, I hacked something together based on wine-staging's related patches, mono's implementation of _CorValidateImage, and disabling a check in wineserver. I'm attaching a diff of that.
Instead of working around managed c++ issues, I looked for a way to use dll fixups more directly from a C# library: https://sites.google.com/site/robertgiesecke/Home/uploads/unmanagedexports
So I have a C# dll that exports an "add" function, like the example, and a win32 console exe that calls it via LoadLibrary/GetProcAddress. On Windows, I can call it multiple times and then exit with no problems. In Wine with your patch, everything works except that the result is incorrect the first time add() is called. I suspect ReallyFixupVTable is changing some registers, and the thunk will have to preserve them.
Thanks for working on this, by the way. Seeing this *almost* work even for a simple test case is very cool. :)
On 01/22/2016 01:32 AM, Vincent Povirk wrote:
In Wine with your patch, everything works except that the result is incorrect the first time add() is called. I suspect ReallyFixupVTable is changing some registers, and the thunk will have to preserve them.
You are right, I've made a dumb mistake forgetting that arguments regs are actually volatile regs and ReallyFixupVTable would not preserve it. I've updated the patch. I want to make a few notes on this: 1. XMM/YMM (0-5) registers save is required to work correctly if MS vectorcall convention is used for method. Though wine & mono itself should not touch SSE regs, app module init is finally called which can do whatever. 2. The modern convention is to align stack on 16 bytes boundary on ms_abi calls, ReallyFixupVTable originally relied on that. I could potentially do a stack alignment in the thunk but I suppose it is not desirable to complicate it further, and so added __attribute__((force_align_arg_pointer)) to declaration (it is not included in CDECL, and probably should not). I realize that some older versions of gcc may somehow ignore this attr (I've read some bug reports on that), but the problem may come only in case our thunk is called unaligned (which potentially could be for some old code, but is not common). If thunk is called correctly it passes the stack pointer aligned. 3. It looks like ReallyFixupVTableEntry has potential thread safety issue. I mean the case when multiple vtable entries are present within one fixup. I could possibly fix that but I think should be an extremely rare case to occur, as application should start calling methods in parallel right from start of module usage at the first place. So I am not sure if it worth doing it.
3. It looks like ReallyFixupVTableEntry has potential thread safety issue. I mean the case when multiple vtable entries are present within one fixup. I could possibly fix that but I think should be an extremely rare case to occur, as application should start calling methods in parallel right from start of module usage at the first place. So I am not sure if it worth doing it.
What's the issue? It should be OK for multiple threads to run it at once.
On 01/22/2016 08:15 PM, Vincent Povirk wrote:
3. It looks like ReallyFixupVTableEntry has potential thread safety issue. I mean the case when multiple vtable entries are present within one fixup. I could possibly fix that but I think should be an extremely rare case to occur, as application should start calling methods in parallel right from start of module usage at the first place. So I am not sure if it worth doing it. What's the issue? It should be OK for multiple threads to run it at once.
I think threads may go marshal the same methods for the same vtable entries with mono_marshal_get_vtfixup_ftnptr (fixup->done is set in the very end only, it is a long time to for some other thread to join), overwriting each others result in vtable. Even if mono is perfectly thread safe attempting init stuff in parallel for a given assembly, I suppose this could lead to loosing the references to the marshalled interfaces and not releasing all of them on DLL unload (though this cleanup might be not implemented yet?). At very most there is a *potential* issue on non-locked multicpu access to fixup->done, though this probably does not worth saying as hitting this in real life is a sort of fantastic. Even if I am not mistaken with this, I still not sure that it is really important as it is very uncommon for the real app to start working with some class multithreaded before calling just any initialization in it.
I actually think it's very important to get theading right, even if it's a "theoretical issue" that "probably won't happen". A managed C++ project probably will not do its runtime initialization from multiple threads, but it could have multiple threads accessing another vtable later. Vtable fixups could also be used to export functions to a mostly unmanaged program that expects them to be thread-safe. I still believe this function is safe, but if it's not then I want to address it. Mono has a lock around the JIT compiler, and it caches the functions it generates. So we can have multiple threads writing the same function pointers into the vtable (which should be fine as long as every other thread that reads it will see either the old value or the new one). Mono doesn't reference-count JITed functions, and I'm not sure if cleaning them up is even possible. Non-locked write to fixup->done is also OK as long as no thread can incorrectly read the value as TRUE and return before the vtable is ready. If a thread incorrectly reads it as FALSE, it'll just do a bit of unnecessary work. Actually, I'm not sure that field is needed at all, it seems like overwriting the values in the vtable should be enough.
On 01/22/2016 09:47 PM, Vincent Povirk wrote:
Mono has a lock around the JIT compiler, and it caches the functions it generates. So we can have multiple threads writing the same function pointers into the vtable (which should be fine as long as every other thread that reads it will see either the old value or the new one). Mono doesn't reference-count JITed functions, and I'm not sure if cleaning them up is even possible.
Oh, I see. So it sounds like there is no clear way to unload the assembly and a DLL with it at all? Do you know if this the case with Win .NET? I am curious because I was trying to locate the issue I get with crash on exit in my test case. I am not filling it yet as bug report as we discussed because it also has MFC (which functions are actually crashing on exit), and I am not sure yet that it is not some issue unrelated to Mono. But this happens only if .Net assembly was loaded during the application run and I suspected that crashed could be triggered by some incompatible unitialization.
Non-locked write to fixup->done is also OK as long as no thread can incorrectly read the value as TRUE and return before the vtable is ready. If a thread incorrectly reads it as FALSE, it'll just do a bit of unnecessary work. Actually, I'm not sure that field is needed at all, it seems like overwriting the values in the vtable should be enough.
I think it is a sort of nearly theoretical question because it would be really very hard to get this in practice. I think if to remove fixup->done it would totally OK as you say. But if not AFAIK it is possible that you get an older vtable value but newer fixup->done if threads are at distinct CPUs (strong enough "fence" is required to guarantee memory access order). BTW I submitted bug report with my test case and patch to mono: https://bugzilla.xamarin.com/show_bug.cgi?id=37913
Oh, I see. So it sounds like there is no clear way to unload the assembly and a DLL with it at all? Do you know if this the case with Win .NET?
I remember hearing that in Mono you can unload an assembly when unloading an appdomain. I'm not sure how that interacts with JITed functions. Also, vtfixup functions end up in the default appdomain, which I believe can only be unloaded at process exit. I don't know if there's a way to do it earlier in native .NET.
I think it is a sort of nearly theoretical question because it would be really very hard to get this in practice. I think if to remove fixup->done it would totally OK as you say. But if not AFAIK it is possible that you get an older vtable value but newer fixup->done if threads are at distinct CPUs (strong enough "fence" is required to guarantee memory access order).
Ah, I think you're right. That seems like a good argument for getting rid of fixup->done.
BTW I submitted bug report with my test case and patch to mono: https://bugzilla.xamarin.com/show_bug.cgi?id=37913
Thanks.
On 22.01.2016 19:47, Vincent Povirk wrote:
I actually think it's very important to get theading right, even if it's a "theoretical issue" that "probably won't happen".
A managed C++ project probably will not do its runtime initialization from multiple threads, but it could have multiple threads accessing another vtable later. Vtable fixups could also be used to export functions to a mostly unmanaged program that expects them to be thread-safe.
I still believe this function is safe, but if it's not then I want to address it.
Mono has a lock around the JIT compiler, and it caches the functions it generates. So we can have multiple threads writing the same function pointers into the vtable (which should be fine as long as every other thread that reads it will see either the old value or the new one). Mono doesn't reference-count JITed functions, and I'm not sure if cleaning them up is even possible.
Non-locked write to fixup->done is also OK as long as no thread can incorrectly read the value as TRUE and return before the vtable is ready. If a thread incorrectly reads it as FALSE, it'll just do a bit of unnecessary work. Actually, I'm not sure that field is needed at all, it seems like overwriting the values in the vtable should be enough.
Besides possible memory leaks of JITed functions, there is no real risk for race-conditions here. Various other parts of Wine also rely on the fact that values up to the size of a pointer can be written with one memory access,
participants (3)
-
Paul Gofman -
Sebastian Lackner -
Vincent Povirk