From 7a95ba4a143c07c3a8a952c8fa522066ba25c496 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 28 May 2019 11:18:28 -0700 Subject: [PATCH] Cleanup unnecessary code in exception formatting (dotnet/coreclr#24797) * Cleanup unnecessary code in exception formatting * Delete fNeedFileInfo argument for GetStackTrace Fixes dotnet/coreclr#8694 Commit migrated from https://github.com/dotnet/coreclr/commit/156240d59b8acc277504fe43f148fe53e63f40bc --- .../src/System/Exception.CoreCLR.cs | 72 ++++++---------------- src/coreclr/src/vm/debughelp.cpp | 35 +++-------- src/coreclr/src/vm/excep.cpp | 22 ++----- src/coreclr/src/vm/mscorlib.h | 1 - .../System.Private.CoreLib/src/System/Exception.cs | 2 +- 5 files changed, 31 insertions(+), 101 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs index a569e01..7597a5b 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs @@ -162,42 +162,27 @@ namespace System { get { - // By default attempt to include file and line number info - return GetStackTrace(true); - } - } + string? stackTraceString = _stackTraceString; + string? remoteStackTraceString = _remoteStackTraceString; - // Computes and returns the stack trace as a string - // Attempts to get source file and line number information if needFileInfo - // is true. Note that this requires FileIOPermission(PathDiscovery), and so - // will usually fail in CoreCLR. To avoid the demand and resulting - // SecurityException we can explicitly not even try to get fileinfo. - private string? GetStackTrace(bool needFileInfo) - { - string? stackTraceString = _stackTraceString; - string? remoteStackTraceString = _remoteStackTraceString; + // if no stack trace, try to get one + if (stackTraceString != null) + { + return remoteStackTraceString + stackTraceString; + } + if (_stackTrace == null) + { + return remoteStackTraceString; + } - // if no stack trace, try to get one - if (stackTraceString != null) - { - return remoteStackTraceString + stackTraceString; + return remoteStackTraceString + GetStackTrace(this); } - if (_stackTrace == null) - { - return remoteStackTraceString; - } - - // Obtain the stack trace string. Note that since GetStackTrace - // will add the path to the source file if the PDB is present: - // we need to make sure we don't store the stack trace string in - // the _stackTraceString member variable. - return remoteStackTraceString + GetStackTrace(needFileInfo, this); } - private static string GetStackTrace(bool needFileInfo, Exception e) + private static string GetStackTrace(Exception e) { // Do not include a trailing newline for backwards compatibility - return new StackTrace(e, needFileInfo).ToString(System.Diagnostics.StackTrace.TraceFormat.Normal); + return new StackTrace(e, fNeedFileInfo: true).ToString(System.Diagnostics.StackTrace.TraceFormat.Normal); } private string? CreateSourceName() @@ -244,30 +229,11 @@ namespace System // copy the stack trace to _remoteStackTraceString. internal void InternalPreserveStackTrace() { - string? tmpStackTraceString; - -#if FEATURE_APPX - if (ApplicationModel.IsUap) - { - // Call our internal GetStackTrace in AppX so we can parse the result should - // we need to strip file/line info from it to make it PII-free. Calling the - // public and overridable StackTrace getter here was probably not intended. - tmpStackTraceString = GetStackTrace(true); - - // Make sure that the _source field is initialized if Source is not overriden. - // We want it to contain the original faulting point. - string? source = Source; - } - else -#else // FEATURE_APPX - // Preinitialize _source on CoreSystem as well. The legacy behavior is not ideal and - // we keep it for back compat but we can afford to make the change on the Phone. + // Make sure that the _source field is initialized if Source is not overriden. + // We want it to contain the original faulting point. string? source = Source; -#endif // FEATURE_APPX - { - // Call the StackTrace getter in classic for compat. - tmpStackTraceString = StackTrace; - } + + string? tmpStackTraceString = StackTrace; if (tmpStackTraceString != null && tmpStackTraceString.Length > 0) { @@ -426,7 +392,7 @@ namespace System if (stackTraceString == null && _stackTrace != null) { - stackTraceString = GetStackTrace(true, this); + stackTraceString = GetStackTrace(this); } return stackTraceString; diff --git a/src/coreclr/src/vm/debughelp.cpp b/src/coreclr/src/vm/debughelp.cpp index 0df43d9..ce54fef 100644 --- a/src/coreclr/src/vm/debughelp.cpp +++ b/src/coreclr/src/vm/debughelp.cpp @@ -773,40 +773,19 @@ void PrintException(OBJECTREF pObjectRef) GCPROTECT_BEGIN(pObjectRef); - if (!IsException(pObjectRef->GetMethodTable())) - { - printf("Specified object is not an exception object.\n"); - } - else - { - MethodDescCallSite toString(METHOD__EXCEPTION__TO_STRING, &pObjectRef); + MethodDescCallSite toString(METHOD__OBJECT__TO_STRING, &pObjectRef); - ARG_SLOT arg[1] = { - ObjToArgSlot(pObjectRef) - }; + ARG_SLOT arg[1] = { + ObjToArgSlot(pObjectRef) + }; - STRINGREF str = toString.Call_RetSTRINGREF(arg); + STRINGREF str = toString.Call_RetSTRINGREF(arg); - if(str->GetBuffer() != NULL) - { - WszOutputDebugString(str->GetBuffer()); - } - } - - GCPROTECT_END(); -} - -void PrintException(UINT_PTR pObject) -{ - CONTRACTL + if(str->GetBuffer() != NULL) { - NOTHROW; - GC_NOTRIGGER; + WszOutputDebugString(str->GetBuffer()); } - CONTRACTL_END; - OBJECTREF pObjectRef = NULL; - GCPROTECT_BEGIN(pObjectRef); GCPROTECT_END(); } diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index e86fbf9..156a8e7 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -339,23 +339,13 @@ STRINGREF GetExceptionMessage(OBJECTREF throwable) if (throwable == NULL) return NULL; - // Assume we're calling Exception.ToString() ... - BinderMethodID sigID = METHOD__EXCEPTION__TO_STRING; - - // ... but if it isn't an exception, call Object.ToString(). - _ASSERTE(IsException(throwable->GetMethodTable())); // what is the pathway here? - if (!IsException(throwable->GetMethodTable())) - { - sigID = METHOD__OBJECT__TO_STRING; - } - // Return value. STRINGREF pString = NULL; GCPROTECT_BEGIN(throwable); - // Get the MethodDesc on which we'll call. - MethodDescCallSite toString(sigID, &throwable); + // Call Object.ToString(). Note that exceptions do not have to inherit from System.Exception + MethodDescCallSite toString(METHOD__OBJECT__TO_STRING, &throwable); // Make the call. ARG_SLOT arg[1] = {ObjToArgSlot(throwable)}; @@ -497,12 +487,8 @@ void ExceptionPreserveStackTrace( // No return. { LOG((LF_EH, LL_INFO1000, "ExceptionPreserveStackTrace called\n")); - // We're calling Exception.InternalPreserveStackTrace() ... - BinderMethodID sigID = METHOD__EXCEPTION__INTERNAL_PRESERVE_STACK_TRACE; - - - // Get the MethodDesc on which we'll call. - MethodDescCallSite preserveStackTrace(sigID, &throwable); + // Call Exception.InternalPreserveStackTrace() ... + MethodDescCallSite preserveStackTrace(METHOD__EXCEPTION__INTERNAL_PRESERVE_STACK_TRACE, &throwable); // Make the call. ARG_SLOT arg[1] = {ObjToArgSlot(throwable)}; diff --git a/src/coreclr/src/vm/mscorlib.h b/src/coreclr/src/vm/mscorlib.h index 13ffd95..442a249 100644 --- a/src/coreclr/src/vm/mscorlib.h +++ b/src/coreclr/src/vm/mscorlib.h @@ -357,7 +357,6 @@ DEFINE_METHOD(EXCEPTION, GET_CLASS_NAME, GetClassName, DEFINE_PROPERTY(EXCEPTION, MESSAGE, Message, Str) DEFINE_PROPERTY(EXCEPTION, SOURCE, Source, Str) DEFINE_PROPERTY(EXCEPTION, HELP_LINK, HelpLink, Str) -DEFINE_METHOD(EXCEPTION, TO_STRING, ToString, IM_RetStr) DEFINE_METHOD(EXCEPTION, INTERNAL_PRESERVE_STACK_TRACE, InternalPreserveStackTrace, IM_RetVoid) #ifdef FEATURE_COMINTEROP DEFINE_METHOD(EXCEPTION, ADD_EXCEPTION_DATA_FOR_RESTRICTED_ERROR_INFO, AddExceptionDataForRestrictedErrorInfo, IM_Str_Str_Str_Obj_Bool_RetVoid) diff --git a/src/libraries/System.Private.CoreLib/src/System/Exception.cs b/src/libraries/System.Private.CoreLib/src/System/Exception.cs index 521ade0..2f07ca2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Exception.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Exception.cs @@ -155,7 +155,7 @@ namespace System " " + SR.Exception_EndOfInnerExceptionStack; } - string? stackTrace = GetStackTrace(needFileInfo: true); + string? stackTrace = StackTrace; if (stackTrace != null) { s += Environment.NewLine + stackTrace; -- 2.7.4