Removing g_hmodCoreCLR and g_hThisInst
#43735
Conversation
|
If this works (and they’re only needed on Windows) then you should be able to use the __ImageBase symbol to replace g_hThisInst and replace the main usage of g_hmodCoreCLR (I think diagnostics uses it in a slightly different way). |
|
It might be worth a code comment explaining that 42 has no meaning other than to be non-zero to save a future reader some confusion. |
|
@Wraith2 - it will definitely be a named constant, not a literal. This was just a quick check to confirm that any nonzero value works. |
VSadov
changed the title
g_hmodCoreCLR and g_hThisInst on Unix.g_hmodCoreCLR and g_hThisInst on Unix.
VSadov
changed the title
g_hmodCoreCLR and g_hThisInst on Unix.g_hmodCoreCLR and g_hThisInst
src/coreclr/src/vm/excep.cpp
Outdated
| @@ -7473,7 +7473,7 @@ VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase3(PEXCEPTION_POINTERS pExcepti | |||
| if ((!fAVisOk) && !(pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)) | |||
| { | |||
| PCODE ip = (PCODE)GetIP(pContext); | |||
| if (IsIPInModule(g_hThisInst, ip) || IsIPInModule(GCHeapUtilities::GetGCModule(), ip)) | |||
| if (IsIPInModule(GetClrModuleBase(), ip) || IsIPInModule(GCHeapUtilities::GetGCModule(), ip)) | |||
There was a problem hiding this comment.
GetGCModule does not return module base on Unix.
Since this IsIPInModule check is Windows-only (at least today) anyway, should we make Windows-only, and ifdef the callsites for Windows?
There was a problem hiding this comment.
GetGCModule returns the module base for normal GC. (it is set in InitializeDefaultGC()).
Standalone variant returns hMod. Is standalone GC supported on Unix?
There was a problem hiding this comment.
I think it may be possible to pass module base for the standalone GC module.
Just wonder if standalone GC on Unix is a real scenario. Maybe should just pass NULL on Unix/standalone case.
There was a problem hiding this comment.
There is nothing that prevents standalone GC from working on Unix.
The non-scenario on Unix is the special handling of crashes in selected modules that this code is for. As far as I know, it works just fine without any special handling. There is no SEH nor exception interop on Unix.
There was a problem hiding this comment.
I made standalone GC to pass the module base as well. I think that minimizes platform differences.
If one day we want to special case crashes in CLR/GC for reasons that include Unix as well, it will have a good chance to work on all platforms.
…ther `GetClrModuleBase`
|
I think I have addressed all the concerns. Please take a look and let me know if something is not adequately resolved. |
| #endif | ||
| InitializeCriticalSection(&g_dacCritSec); | ||
|
|
||
| #else | ||
| // Save the module handle. | ||
| g_thisModule = (HINSTANCE)instance; |
There was a problem hiding this comment.
Can g_thisModule be deleted now? (Or if it needs to stay - ifdef it out for non-Windows at the top of the file?)
There was a problem hiding this comment.
Yes, of course. This assignment is the only use.
I thought I removed them all. This one does not have "h" in the name could be missed when I searched for what remains.
| return S_OK; | ||
| #else | ||
| // hModule is not avaialable under TARGET_UNIX |
There was a problem hiding this comment.
A nit:
| // hModule is not avaialable under TARGET_UNIX | |
| // hModule is not available under TARGET_UNIX |
There was a problem hiding this comment.
There was one more typo like this. Will fix both.
|
Thanks!!! |
|
Hello @VSadov! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
ghost
locked as resolved and limited conversation to collaborators
|
I think that the simplest fix is to revert the part of this change that introduced the problem. Something like main...jkotas:runtime:thrown-by-us . I have not tested it. |
Introduced two new internal APIs:
Unlike
g_hmodCoreCLRandg_hThisInst, these methods make sense and can be implemented regardless of the platform.They also do not need to rely on
DllMainor other kind of initialization.All uses of
g_hmodCoreCLRandg_hThisInstin coreclr were reduced to the above APIs and the variables have been removed.