Fix setting of DllImportResolver in crossgen2. #167
Conversation
src/coreclr/src/tools/crossgen2/Common/JitInterface/CorInfoImpl.cs
Outdated
Show resolved
Hide resolved
src/coreclr/src/tools/crossgen2/Common/JitInterface/CorInfoImpl.cs
Outdated
Show resolved
Hide resolved
|
I pushed a change that ensures that the resolver is registered before we attempt to load the jit. I verified that with this change --jitpath option works correctly both on Windows and Linux. |
|
@jkotas PTAL |
src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs
Show resolved
Hide resolved
| { | ||
| return true; | ||
| if (PAL_RegisterModule("libclrjitilc.so") == (IntPtr)1) |
There was a problem hiding this comment.
| if (PAL_RegisterModule("libclrjitilc.so") == (IntPtr)1) | |
| if (PAL_RegisterModule("libclrjitilc.so") == IntPtr.Zero) |
As far as I can tell, PAL_RegisterModule returns 0 on failure.
| { | ||
| return true; | ||
| if (PAL_RegisterModule("libclrjitilc.so") == IntPtr.Zero) |
There was a problem hiding this comment.
PAL_RegisterModule does not use the DllImport resolver, so I think you have to pass the full path to it.
There was a problem hiding this comment.
This is hard/impossible to make it work 100% of time. It may be best to ignore the errors from PAL_RegisterModule.
The whole PAL_RegisterModule scheme is pretty broken. I would be best to not have it. The proper way to fix this would be to delete PAL_RegisterModule export from the JIT, and do the work that it does inside jitStartup instead.
There was a problem hiding this comment.
I removed the check for errors from PAL_RegisterModule. I think moving the call to PAL_InitializeDll to jitStartup should be a separate change as it affects all the callers of jitStartup.
There was a problem hiding this comment.
Could you please add a comment and link to cleanup issue for this?
| if (_jitConfig.JitPath != null) | ||
| { | ||
| NativeLibrary.SetDllImportResolver(typeof(CorInfoImpl).Assembly, JitLibraryResolver); | ||
| } | ||
|
|
||
| jitStartup(GetJitHost(_jitConfig.UnmanagedInstance)); |
There was a problem hiding this comment.
This should be insider the register method as well. It is not a good idea to call it multiple time.
|
It would be nice if you can at least move the call to |
After change dotnet/coreclr#27068 we are creating multiple instances of CorInfoImpl. That broke the scenario when jitpath is set: we are calling NativeLibrary.SetDllImportResolver more than once, which results in `System.InvalidOperationException: A resolver is already set for the assembly.` This fix ensures that we call NativeLibrary.SetDllImportResolver at most once. This change also ensures that we set the resolver before attempting to load the jit. That fixes the --jitpath scenario on Linux.
Done. |
|
@jkotas Are you ok with the current version? |
ghost
locked as resolved and limited conversation to collaborators
After change dotnet/coreclr#27068 we
are creating multiple instances of CorInfoImpl. That broke the scenario
when jitpath is set: we are calling NativeLibrary.SetDllImportResolver
more than once, which results in
System.InvalidOperationException: A resolver is already set for the assembly.This fix ensures that we call NativeLibrary.SetDllImportResolver
at most once.