Adjust additional source under utilcode and vm to use minipal_log API. #113916
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 23 changed files in this pull request and generated no comments.
Files not reviewed (20)
- src/coreclr/utilcode/debug.cpp: Language not supported
- src/coreclr/utilcode/executableallocator.cpp: Language not supported
- src/coreclr/utilcode/loaderheap.cpp: Language not supported
- src/coreclr/utilcode/util_nodependencies.cpp: Language not supported
- src/coreclr/vm/assembly.cpp: Language not supported
- src/coreclr/vm/binder.cpp: Language not supported
- src/coreclr/vm/ceemain.cpp: Language not supported
- src/coreclr/vm/class.cpp: Language not supported
- src/coreclr/vm/classcompat.cpp: Language not supported
- src/coreclr/vm/codeman.h: Language not supported
- src/coreclr/vm/codepitchingmanager.cpp: Language not supported
- src/coreclr/vm/debughelp.cpp: Language not supported
- src/coreclr/vm/disassembler.cpp: Language not supported
- src/coreclr/vm/ecall.cpp: Language not supported
- src/coreclr/vm/eetwain.cpp: Language not supported
- src/coreclr/vm/frames.cpp: Language not supported
- src/coreclr/vm/gchelpers.cpp: Language not supported
- src/coreclr/vm/gdbjit.cpp: Language not supported
- src/coreclr/vm/hash.cpp: Language not supported
- src/coreclr/vm/jithelpers.cpp: Language not supported
|
Tagging subscribers to this area: @mangod9 |
|
Failures in [runtime "Build Libraries Test Run checked coreclr linux x64 Release" appears on other PR's as well, so unrelated. |
| fprintf(stderr, "Map create time sum: %lldms\n", g_mapCreateTimeSum / (freq.QuadPart / 1000)); | ||
| fprintf(stderr, "Unmap time with lock sum: %lldms\n", g_unmapTimeWithLockSum / (freq.QuadPart / 1000)); | ||
| fprintf(stderr, "Unmap time sum: %lldms\n", g_unmapTimeSum / (freq.QuadPart / 1000)); | ||
| minipal_log_print_error("Map time with lock sum: %lldms\n", g_mapTimeWithLockSum / (freq.QuadPart / 1000)); |
There was a problem hiding this comment.
These are not errors, I've just let it go to the stderr for no apparent reason. This stuff is not called unless you manually uncomment //#define LOG_EXECUTABLE_ALLOCATOR_STATISTICS in the executableallocator.h. It is really meant just a helper for performance investigation by developers. I have used it when I was implementing the executable allocator and left it in just in case we would want to investigate the performance e.g. on some new platforms.
So let's log that as info instead.
| @@ -251,7 +251,7 @@ HRESULT ExecutableAllocator::StaticInitialize(FatalErrorHandler fatalErrorHandle | |||
| { | |||
| if ((customCacheSize > ARRAY_SIZE(m_cachedMapping)) || (customCacheSize <= 0)) | |||
| { | |||
| printf("Invalid value in 'EXECUTABLE_ALLOCATOR_CACHE_SIZE' environment variable'\n"); | |||
| minipal_log_print_info("Invalid value in 'EXECUTABLE_ALLOCATOR_CACHE_SIZE' environment variable'\n"); | |||
There was a problem hiding this comment.
This should go to error instead.
| @@ -1228,8 +1228,7 @@ static void RunMainInternal(Param* pParam) | |||
|
|
|||
| //<TODO> | |||
| // When we get mainCRTStartup from the C++ then this should be able to go away.</TODO> | |||
There was a problem hiding this comment.
Can you please remove the TODO comment? It has been there even before .NET Core 1.0 and I don't think we will ever do anything with it.
| @@ -898,7 +898,7 @@ static void FCallCheckSignature(MethodDesc* pMD, PCODE pImpl) | |||
| // when managed type is well known | |||
| if (!(strlen(expectedType) == len && SString::_strnicmp(expectedType, pUnmanagedArg, (COUNT_T)len) == 0)) | |||
| { | |||
| printf("CheckExtended: The managed and unmanaged fcall signatures do not match, Method: %s:%s. Argument: %d Expecting: %s Actual: %s\n", pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, argIndex, expectedType, pUnManagedType); | |||
| minipal_log_print_info("CheckExtended: The managed and unmanaged fcall signatures do not match, Method: %s:%s. Argument: %d Expecting: %s Actual: %s\n", pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, argIndex, expectedType, pUnManagedType); | |||
There was a problem hiding this comment.
I think it would make sense to direct these to error even though they were being sent to stdout before. I think with this kind of logging, people were choosing whether to direct it to stderr or stdout kind of randomly for stuff in debug builds that is not customer facing.
| @@ -2431,8 +2431,7 @@ class ExecutionManager | |||
|
|
|||
| static void DumpExecutionManagerUsage() | |||
| { | |||
| fprintf(stderr, "JumpStub usage count:\n"); | |||
| fprintf(stderr, "Normal: %u, LCG: %u\n", m_normal_JumpStubLookup, m_LCG_JumpStubLookup); | |||
| minipal_log_print_error("JumpStub usage count:\nNormal: %u, LCG: %u\n", m_normal_JumpStubLookup, m_LCG_JumpStubLookup); | |||
There was a problem hiding this comment.
Please make this info, it is not an error.
| @@ -67,7 +67,7 @@ bool Disassembler::IsAvailable() | |||
| } | |||
|
|
|||
| #if _DEBUG | |||
| #define DISPLAYERROR(FMT, ...) printf(FMT, __VA_ARGS__) | |||
| #define DISPLAYERROR(FMT, ...) minipal_log_print_info(FMT, __VA_ARGS__) | |||
| @@ -2255,7 +2259,7 @@ bool CheckGCRefMapEqual(PTR_BYTE pGCRefMap, MethodDesc* pMD, bool isDispatchCell | |||
| } | |||
| if (invalidGCRefMap) | |||
| { | |||
| printf("GC ref map mismatch detected for method: %s::%s\n", pMD->GetMethodTable()->GetDebugClassName(), pMD->GetName()); | |||
| minipal_log_print_info("GC ref map mismatch detected for method: %s::%s\n", pMD->GetMethodTable()->GetDebugClassName(), pMD->GetName()); | |||
There was a problem hiding this comment.
Can you please direct this to error?
| @@ -238,7 +238,7 @@ TypeInfoBase* GetLocalTypeInfo(MethodDesc *methodDescPtr, | |||
|
|
|||
| if (FAILED(methodDescPtr->GetMDImport()->GetSigFromToken(method.GetLocalVarSigTok(), &cbSigLen, &pComSig))) | |||
| { | |||
| printf("\nInvalid record"); | |||
| minipal_log_print_info("\nInvalid record"); | |||
| @@ -13588,7 +13588,7 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob, BOOL printDiff) | |||
| result = FALSE; | |||
|
|
|||
| DefineFullyQualifiedNameForClass(); | |||
| printf("Type %s: expected alignment 0x%08x, actual 0x%08x\n", | |||
| minipal_log_print_info("Type %s: expected alignment 0x%08x, actual 0x%08x\n", | |||
There was a problem hiding this comment.
I would direct the messages in this method to error.
|
@janvorli Thanks for review, I did change existing logging since I didn't have too much experience of their usage, so primarily just did a straight adaption so if it used stdout it was ported to info and if it used strerr it was ported to error. I will go over your comment and adjust loggings based on feedback. Thanks! |
#113416 established a logging API in native/minipal wired up to work on platforms not supporting standard stdout/stderr, like Android. That initial PR changed a couple of locations from regular printf's to minipal_log calls.
This follow-up PR adjust more locations inside coreclr utilcode and vm folder using minipal_log API's for logging. There are still a couple of places that have not been adjusted, mainly due to unactive code (commented out or #if 0) and the code in src\coreclr\vm\gc_unwind_x86.inl was intestinally left out.
There are more places that could be adjusted to use minipal_log API's instead of straight printf's, like the pal, nativeaot, but going forward, this could be done on a case-by-case basis, if the specific logging is needed on platforms without working stdout/stderr.