From aaac67d2afed1afbb2fbbeb31502b83f4275ab1c Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 6 Jan 2020 22:04:39 -0800 Subject: [PATCH 1/4] Fix line number issues in SOS The change is to subtract 1 from the IP used to get the source/line info when the stack frame is a leaf node. For hardware exceptions, the IP has already been adjusted (+1) by the runtime so they cancel out. Fixes in SOS the same issues reported in https://github.com/dotnet/coreclr/issues/27765 and https://github.com/dotnet/coreclr/issues/25740 for the runtime's unhandled exception message. Add some more PrintException -lines testing for thrown and hardware exceptions. Gracefully fail with runtime module size == 0. Fix "clrmodules" failure our vendors found. --- src/SOS/SOS.NETCore/SymbolReader.cs | 3 +- src/SOS/SOS.UnitTests/Scripts/DivZero.script | 17 +++++- .../SOS.UnitTests/Scripts/SimpleThrow.script | 14 ++++- src/SOS/Strike/disasm.h | 9 ++- src/SOS/Strike/exts.h | 2 + src/SOS/Strike/runtime.cpp | 12 +++- src/SOS/Strike/strike.cpp | 57 +++++++++++++++++-- src/SOS/Strike/util.cpp | 6 +- src/SOS/Strike/util.h | 2 +- src/Tools/dotnet-dump/Analyzer.cs | 7 ++- 10 files changed, 114 insertions(+), 15 deletions(-) diff --git a/src/SOS/SOS.NETCore/SymbolReader.cs b/src/SOS/SOS.NETCore/SymbolReader.cs index 50231e29ba..1f17a22843 100644 --- a/src/SOS/SOS.NETCore/SymbolReader.cs +++ b/src/SOS/SOS.NETCore/SymbolReader.cs @@ -19,6 +19,7 @@ using System.Reflection.PortableExecutable; using System.Runtime.InteropServices; using System.Threading; +using System.Threading.Tasks; namespace SOS { @@ -309,7 +310,7 @@ public static void LoadNativeSymbols(SymbolFileCallback callback, IntPtr paramet } } } - catch (Exception ex) when (ex is BadInputFormatException || ex is InvalidVirtualAddressException) + catch (Exception ex) when (ex is BadInputFormatException || ex is InvalidVirtualAddressException || ex is TaskCanceledException) { s_tracer.Error("{0}/{1:X16}: {2}", moduleFilePath, address, ex.Message); } diff --git a/src/SOS/SOS.UnitTests/Scripts/DivZero.script b/src/SOS/SOS.UnitTests/Scripts/DivZero.script index 47379e38de..be215a4a99 100644 --- a/src/SOS/SOS.UnitTests/Scripts/DivZero.script +++ b/src/SOS/SOS.UnitTests/Scripts/DivZero.script @@ -10,7 +10,6 @@ LOADSOS # Verifying that PrintException gives us the right exception in the format above. SOSCOMMAND:PrintException - VERIFY:Exception object:\s+\s+ VERIFY:Exception type:\s+System\.DivideByZeroException\s+ VERIFY:Message:\s+(|Attempted to divide by zero\.)\s+ @@ -24,6 +23,22 @@ VERIFY:\s+\s+\s+[Dd]iv[Zz]ero.*!C\.Main(\(.*\))?\+0x\s+ VERIFY:(StackTraceString: \s+)?\s+ VERIFY:HResult:\s+80020012\s+ +SOSCOMMAND:PrintException -lines +VERIFY:Exception object:\s+\s+ +VERIFY:Exception type:\s+System\.DivideByZeroException\s+ +VERIFY:Message:\s+(|Attempted to divide by zero\.)\s+ +VERIFY:InnerException:\s+\s+ +VERIFY:StackTrace \(generated\):\s+ +VERIFY:\s+SP\s+IP\s+\s+Function\s+ +VERIFY:\s+\s+\s+[Dd]iv[Zz]ero.*!C\.DivideByZero(\(.*\))?\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?DivZero[\\|/]DivZero\.cs @ 12\s*\]\s* +VERIFY:\s+\s+\s+[Dd]iv[Zz]ero.*!C\.F3(\(.*\))?\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?DivZero[\\|/]DivZero\.cs @ 21\s*\]\s* +VERIFY:\s+\s+\s+[Dd]iv[Zz]ero.*!C\.F2(\(.*\))?\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?DivZero[\\|/]DivZero\.cs @ 33\s*\]\s* +VERIFY:\s+\s+\s+[Dd]iv[Zz]ero.*!C\.Main(\(.*\))?\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?DivZero[\\|/]DivZero\.cs @ 54\s*\]\s* + # Verify that Threads (clrthreads) works IFDEF:DOTNETDUMP SOSCOMMAND:clrthreads diff --git a/src/SOS/SOS.UnitTests/Scripts/SimpleThrow.script b/src/SOS/SOS.UnitTests/Scripts/SimpleThrow.script index 278a45f3da..c2bf379c88 100644 --- a/src/SOS/SOS.UnitTests/Scripts/SimpleThrow.script +++ b/src/SOS/SOS.UnitTests/Scripts/SimpleThrow.script @@ -33,6 +33,18 @@ VERIFY:\s+\s+\s+[Ss]imple[Tt]hrow.*!(\$0_)?UserObject\.UseObject VERIFY:\s+\s+\s+[Ss]imple[Tt]hrow.*!(\$0_)?Simple\.Main.*\+0x\s+ VERIFY:HResult:\s+80131509 +SOSCOMMAND:PrintException -lines +VERIFY:Exception object:\s+\s+ +VERIFY:Exception type:\s+System\.InvalidOperationException\s+ +VERIFY:Message:\s+(|Throwing an invalid operation\.\.\.\.)\s+ +VERIFY:InnerException:\s+\s+ +VERIFY:StackTrace\s+\(generated\):\s+ +VERIFY:\s+SP\s+IP\s+Function\s+ +VERIFY:\s+\s+\s+[Ss]imple[Tt]hrow.*!(\$0_)?UserObject\.UseObject.*\+0x\s* +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?SimpleThrow[\\|/]UserObject\.cs @ 16\s*\]\s* +VERIFY:\s+\s+\s+[Ss]imple[Tt]hrow.*!(\$0_)?Simple\.Main.*\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?SimpleThrow[\\|/]SimpleThrow\.cs @ 9\s*\]\s* + # Verify that Threads (clrthreads) works IFDEF:DOTNETDUMP SOSCOMMAND:clrthreads @@ -54,6 +66,6 @@ SOSCOMMAND:ClrStack VERIFY:.*OS Thread Id:\s+0x\s+.* VERIFY:\s+Child\s+SP\s+IP\s+Call Site\s+ VERIFY:\s+\s+\s+(\*\*\* WARNING: Unable to verify checksum for SimpleThrow.exe\s*)?UserObject\.UseObject.*\s+ -VERIFY:[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?SimpleThrow[\\|/]UserObject\.cs @ 18\s*\] +VERIFY:[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?SimpleThrow[\\|/]UserObject\.cs @ 16\s*\] VERIFY:\s+\s+\s+Simple\.Main\(\)\s+ VERIFY:[.*[\\|/]Debuggees[\\|/](dotnet.+[\\|/])?SimpleThrow[\\|/]SimpleThrow\.cs @ 9\s*\] diff --git a/src/SOS/Strike/disasm.h b/src/SOS/Strike/disasm.h index 1eb3920cb0..fb2ee3e640 100644 --- a/src/SOS/Strike/disasm.h +++ b/src/SOS/Strike/disasm.h @@ -125,7 +125,6 @@ eTargetType GetFinalTarget(DWORD_PTR callee, DWORD_PTR* finalMDorIP); #ifndef THUMB_CODE #define THUMB_CODE 1 #endif -#define STACKWALK_CONTROLPC_ADJUST_OFFSET 2 #ifdef SOS_TARGET_X86 @@ -177,6 +176,8 @@ class X86Machine : public IMachine virtual void DumpGCInfo(GCInfoToken gcInfoToken, unsigned methodSize, printfFtn gcPrintf, bool encBytes, bool bPrintHeader) const; + int StackWalkIPAdjustOffset() const { return 1; } + private: X86Machine() {} ~X86Machine() {} @@ -244,6 +245,8 @@ class ARMMachine : public IMachine virtual void DumpGCInfo(GCInfoToken gcInfoToken, unsigned methodSize, printfFtn gcPrintf, bool encBytes, bool bPrintHeader) const; + int StackWalkIPAdjustOffset() const { return 2; } + private: ARMMachine() {} ~ARMMachine() {} @@ -313,6 +316,8 @@ class AMD64Machine : public IMachine virtual void DumpGCInfo(GCInfoToken gcInfoToken, unsigned methodSize, printfFtn gcPrintf, bool encBytes, bool bPrintHeader) const; + int StackWalkIPAdjustOffset() const { return 1; } + private: AMD64Machine() {} ~AMD64Machine() {} @@ -378,6 +383,8 @@ class ARM64Machine : public IMachine virtual void DumpGCInfo(GCInfoToken gcInfoToken, unsigned methodSize, printfFtn gcPrintf, bool encBytes, bool bPrintHeader) const; + int StackWalkIPAdjustOffset() const { return 4; } + private: ARM64Machine() {} ~ARM64Machine() {} diff --git a/src/SOS/Strike/exts.h b/src/SOS/Strike/exts.h index cefec80d91..2d630b87bd 100644 --- a/src/SOS/Strike/exts.h +++ b/src/SOS/Strike/exts.h @@ -390,6 +390,8 @@ class IMachine typedef void (*printfFtn)(const char* fmt, ...); // Dumps the GCInfo virtual void DumpGCInfo(GCInfoToken gcInfoToken, unsigned methodSize, printfFtn gcPrintf, bool encBytes, bool bPrintHeader) const = 0; + // The amount of bytes to adjust the IP for software exception throw instructions (the STACKWALK_CONTROLPC_ADJUST_OFFSET define in the runtime) + virtual int StackWalkIPAdjustOffset() const = 0; protected: IMachine() {} diff --git a/src/SOS/Strike/runtime.cpp b/src/SOS/Strike/runtime.cpp index cafe21bd1c..04d48bfa09 100644 --- a/src/SOS/Strike/runtime.cpp +++ b/src/SOS/Strike/runtime.cpp @@ -85,8 +85,16 @@ HRESULT Runtime::CreateInstance(bool isDesktop, Runtime **ppRuntime) } if (SUCCEEDED(hr)) { - *ppRuntime = new Runtime(isDesktop, moduleIndex, moduleAddress, moduleSize); - OnUnloadTask::Register(CleanupRuntimes); + if (moduleSize > 0) + { + *ppRuntime = new Runtime(isDesktop, moduleIndex, moduleAddress, moduleSize); + OnUnloadTask::Register(CleanupRuntimes); + } + else + { + ExtOut("Runtime (%s) module size == 0\n", runtimeModuleName); + hr = E_INVALIDARG; + } } } return hr; diff --git a/src/SOS/Strike/strike.cpp b/src/SOS/Strike/strike.cpp index 27814e649a..973ec62cb2 100644 --- a/src/SOS/Strike/strike.cpp +++ b/src/SOS/Strike/strike.cpp @@ -2384,13 +2384,15 @@ size_t FormatGeneratedException (DWORD_PTR dataPtr, UINT bytes, __out_ecount_opt(bufferLength) WCHAR *wszBuffer, size_t bufferLength, - BOOL bAsync, + BOOL bAsync, // hardware exception if true BOOL bNestedCase = FALSE, BOOL bLineNumbers = FALSE) { UINT count = bytes / sizeof(StackTraceElement); size_t Length = 0; + _ASSERTE(g_targetMachine != nullptr); + if (wszBuffer && bufferLength > 0) { wszBuffer[0] = L'\0'; @@ -2464,7 +2466,27 @@ size_t FormatGeneratedException (DWORD_PTR dataPtr, WCHAR filename[MAX_LONGPATH] = W(""); ULONG linenum = 0; if (bLineNumbers && - SUCCEEDED(GetLineByOffset(TO_CDADDR(ste.ip), &linenum, filename, _countof(filename)))) + // To get the source line number of the actual code that threw an exception, the IP needs + // to be adjusted in certain cases. + // + // The IP of the leaf node stack frame (i == 0) points to either: + // + // 1) The instruction that caused a hardware exception (div by zero, null ref, etc). + // 2) After the call instruction to an internal runtime function (FCALL like IL_Throw, + // IL_Rethrow, JIT_OverFlow, etc.) that caused a software exception. + // + // It is case 2 that needs to adjust IP because these IL calls don't contain any other instructions + // after and point to next IL instruction and source line after the throw. We distinguish between #1 + // and #2 by the bAsync flag which is set to true for hardware exceptions. + // + // The rest of the stack frame IPs also point to the instruction after the call. The difference + // in this case is that these IL calls consist of more code after the call (at the minimum a nop + // sequence point) that is consider part of the IL instruction. The IP doesn't need to be adjusted + // in these cases. + // + // When the IP needs to be adjusted it is a lot simpler to decrement IP instead of trying to figure + // out the beginning of the instruction. It is enough for GetLineByOffset to return the correct line number. + SUCCEEDED(GetLineByOffset(TO_CDADDR((i == 0 && !bAsync) ? ste.ip - g_targetMachine->StackWalkIPAdjustOffset() : ste.ip), &linenum, filename, _countof(filename)))) { swprintf_s(wszLineBuffer, _countof(wszLineBuffer), W(" %s [%s @ %d]\n"), so.String(), filename, linenum); } @@ -13439,6 +13461,8 @@ class ClrStackImpl public: static void PrintThread(ULONG osID, BOOL bParams, BOOL bLocals, BOOL bSuppressLines, BOOL bGC, BOOL bFull, BOOL bDisplayRegVals) { + _ASSERTE(g_targetMachine != nullptr); + // Symbols variables ULONG symlines = 0; // symlines will be non-zero only if SYMOPT_LOAD_LINES was set in the symbol options if (!bSuppressLines && SUCCEEDED(g_ExtSymbols->GetSymbolOptions(&symlines))) @@ -13481,7 +13505,9 @@ class ClrStackImpl TableOutput out(3, POINTERSIZE_HEX, AlignRight); out.WriteRow("Child SP", "IP", "Call Site"); - + + int frameNumber = 0; + int internalFrames = 0; do { if (IsInterrupt()) @@ -13516,6 +13542,8 @@ class ClrStackImpl // Print the method/Frame info if (SUCCEEDED(frameDataResult) && FrameData.frameAddr) { + internalFrames++; + // Skip the instruction pointer because it doesn't really mean anything for method frames out.WriteColumn(1, bFull ? String("") : NativePtr(ip)); @@ -13534,8 +13562,29 @@ class ClrStackImpl } else { + // To get the source line number of the actual code that threw an exception, the IP needs + // to be adjusted in certain cases. + // + // The IP of leaf node stack frame (frameNumber == 0) points to either: + // + // 1) Currently executing instruction (if you hit a breakpoint or are single stepping through). + // 2) The instruction that caused a hardware exception (div by zero, null ref, etc). + // 3) After the call to an internal runtime function (FCALL like IL_Throw, IL_Rethrow, + // JIT_OverFlow, etc.) that caused a software exception. + // + // It is case 3 that needs to adjust IP because these IL calls don't contain any other instructions + // after and point to next IL instruction and source line after the throw. We distinguish this from + // #1 or #2 by the present of an internal stack frame (usually HelperMethodFrame). + // + // The rest of the stack frame IPs also point to the instruction after the call. The difference + // in this case is that these IL calls consist of more code after the call (at the minimum a nop + // sequence point) that is consider part of the IL instruction. The IP doesn't need to be adjusted + // in these cases. + bool bAdjustIPForLineNumber = frameNumber == 0 && internalFrames > 0; + frameNumber++; + out.WriteColumn(1, InstructionPtr(ip)); - out.WriteColumn(2, MethodNameFromIP(ip, bSuppressLines, bFull, bFull)); + out.WriteColumn(2, MethodNameFromIP(ip, bSuppressLines, bFull, bFull, bAdjustIPForLineNumber)); // Print out gc references. refCount will be zero if bGC is false (or if we // failed to fetch gc reference information). diff --git a/src/SOS/Strike/util.cpp b/src/SOS/Strike/util.cpp index 4bde52925d..6cdae98c51 100644 --- a/src/SOS/Strike/util.cpp +++ b/src/SOS/Strike/util.cpp @@ -5169,7 +5169,7 @@ WString GetFrameFromAddress(TADDR frameAddr, IXCLRDataStackWalk *pStackWalk, BOO return frameOutput; } -WString MethodNameFromIP(CLRDATA_ADDRESS ip, BOOL bSuppressLines, BOOL bAssemblyName, BOOL bDisplacement) +WString MethodNameFromIP(CLRDATA_ADDRESS ip, BOOL bSuppressLines, BOOL bAssemblyName, BOOL bDisplacement, BOOL bAdjustIPForLineNumber) { ULONG linenum; WString methodOutput; @@ -5242,7 +5242,9 @@ WString MethodNameFromIP(CLRDATA_ADDRESS ip, BOOL bSuppressLines, BOOL bAssembly ArrayHolder wszFileName = new WCHAR[MAX_LONGPATH]; if (!bSuppressLines && - SUCCEEDED(GetLineByOffset(TO_CDADDR(ip), &linenum, wszFileName, MAX_LONGPATH))) + // If the IP needs to be adjusted, it is a lot simpler to decrement IP instead of trying to figure out + // the beginning of the instruction. It is enough for GetLineByOffset to return the correct line number. + SUCCEEDED(GetLineByOffset(TO_CDADDR(bAdjustIPForLineNumber ? ip - g_targetMachine->StackWalkIPAdjustOffset() : ip), &linenum, wszFileName, MAX_LONGPATH))) { methodOutput += WString(W(" [")) + wszFileName + W(" @ ") + Decimal(linenum) + W("]"); } diff --git a/src/SOS/Strike/util.h b/src/SOS/Strike/util.h index 92666b8a15..28b6cccda1 100644 --- a/src/SOS/Strike/util.h +++ b/src/SOS/Strike/util.h @@ -2589,7 +2589,7 @@ typedef struct _CROSS_PLATFORM_CONTEXT { WString BuildRegisterOutput(const SOSStackRefData &ref, bool printObj = true); -WString MethodNameFromIP(CLRDATA_ADDRESS methodDesc, BOOL bSuppressLines = FALSE, BOOL bAssemblyName = FALSE, BOOL bDisplacement = FALSE); +WString MethodNameFromIP(CLRDATA_ADDRESS methodDesc, BOOL bSuppressLines = FALSE, BOOL bAssemblyName = FALSE, BOOL bDisplacement = FALSE, BOOL bAdjustIPForLineNumber = FALSE); HRESULT GetGCRefs(ULONG osID, SOSStackRefData **ppRefs, unsigned int *pRefCnt, SOSStackRefError **ppErrors, unsigned int *pErrCount); WString GetFrameFromAddress(TADDR frameAddr, IXCLRDataStackWalk *pStackwalk = NULL, BOOL bAssemblyName = FALSE); diff --git a/src/Tools/dotnet-dump/Analyzer.cs b/src/Tools/dotnet-dump/Analyzer.cs index b48a758102..339901db54 100644 --- a/src/Tools/dotnet-dump/Analyzer.cs +++ b/src/Tools/dotnet-dump/Analyzer.cs @@ -183,7 +183,10 @@ private ClrRuntime CreateRuntime(DataTarget target) string dacFilePath = GetDacFile(clrInfo); try { - runtime = clrInfo.CreateRuntime(dacFilePath); + // Ignore the DAC version mismatch that can happen on Linux because the clrmd ELF dump + // reader returns 0.0.0.0 for the runtime module that the DAC is matched against. This + // will be fixed in clrmd 2.0 but not 1.1. + runtime = clrInfo.CreateRuntime(dacFilePath, ignoreMismatch: RuntimeInformation.IsOSPlatform(OSPlatform.Linux)); } catch (DllNotFoundException ex) { @@ -240,7 +243,7 @@ private string GetDacFile(ClrInfo clrInfo) if (_dacFilePath == null) { - throw new FileNotFoundException("Could not find matching DAC for this runtime: {0}", clrInfo.ModuleInfo.FileName); + throw new FileNotFoundException($"Could not find matching DAC for this runtime: {clrInfo.ModuleInfo.FileName}"); } _isDesktop = clrInfo.Flavor == ClrFlavor.Desktop; } From b08cb8dee7c0a14fb9297cca20ad0ec24daffed7 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 13 Jan 2020 16:49:19 -0800 Subject: [PATCH 2/4] Code review feedback --- src/SOS/Strike/strike.cpp | 44 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/SOS/Strike/strike.cpp b/src/SOS/Strike/strike.cpp index 973ec62cb2..197a167d1e 100644 --- a/src/SOS/Strike/strike.cpp +++ b/src/SOS/Strike/strike.cpp @@ -2469,24 +2469,24 @@ size_t FormatGeneratedException (DWORD_PTR dataPtr, // To get the source line number of the actual code that threw an exception, the IP needs // to be adjusted in certain cases. // - // The IP of the leaf node stack frame (i == 0) points to either: + // The IP of the stack frame points to either: // // 1) The instruction that caused a hardware exception (div by zero, null ref, etc). - // 2) After the call instruction to an internal runtime function (FCALL like IL_Throw, + // 2) The instruction after the call to an internal runtime function (FCALL like IL_Throw, // IL_Rethrow, JIT_OverFlow, etc.) that caused a software exception. + // 3) The instruction after the call to a managed function (non-leaf node). // - // It is case 2 that needs to adjust IP because these IL calls don't contain any other instructions - // after and point to next IL instruction and source line after the throw. We distinguish between #1 - // and #2 by the bAsync flag which is set to true for hardware exceptions. - // - // The rest of the stack frame IPs also point to the instruction after the call. The difference - // in this case is that these IL calls consist of more code after the call (at the minimum a nop - // sequence point) that is consider part of the IL instruction. The IP doesn't need to be adjusted - // in these cases. + // #2 and #3 are the cases that need to adjust IP because they point after the call instruction + // and may point to the next (incorrect) IL instruction/source line. We distinguish these from + // #1 by the bAsync flag which is set to true for hardware exceptions. // // When the IP needs to be adjusted it is a lot simpler to decrement IP instead of trying to figure // out the beginning of the instruction. It is enough for GetLineByOffset to return the correct line number. - SUCCEEDED(GetLineByOffset(TO_CDADDR((i == 0 && !bAsync) ? ste.ip - g_targetMachine->StackWalkIPAdjustOffset() : ste.ip), &linenum, filename, _countof(filename)))) + // + // The unmodified IP is displayed (above by DumpMDInfoBuffer) which points after the exception in most + // cases. This means that the printed IP and the printed line number often will not map to one another + // and this is intentional. + SUCCEEDED(GetLineByOffset(TO_CDADDR(bAsync ? ste.ip : ste.ip - g_targetMachine->StackWalkIPAdjustOffset()), &linenum, filename, _countof(filename)))) { swprintf_s(wszLineBuffer, _countof(wszLineBuffer), W(" %s [%s @ %d]\n"), so.String(), filename, linenum); } @@ -13565,24 +13565,22 @@ class ClrStackImpl // To get the source line number of the actual code that threw an exception, the IP needs // to be adjusted in certain cases. // - // The IP of leaf node stack frame (frameNumber == 0) points to either: + // The IP of stack frame points to either: // // 1) Currently executing instruction (if you hit a breakpoint or are single stepping through). // 2) The instruction that caused a hardware exception (div by zero, null ref, etc). - // 3) After the call to an internal runtime function (FCALL like IL_Throw, IL_Rethrow, + // 3) The instruction after the call to an internal runtime function (FCALL like IL_Throw, // JIT_OverFlow, etc.) that caused a software exception. + // 4) The instruction after the call to a managed function (non-leaf node). // - // It is case 3 that needs to adjust IP because these IL calls don't contain any other instructions - // after and point to next IL instruction and source line after the throw. We distinguish this from - // #1 or #2 by the present of an internal stack frame (usually HelperMethodFrame). - // - // The rest of the stack frame IPs also point to the instruction after the call. The difference - // in this case is that these IL calls consist of more code after the call (at the minimum a nop - // sequence point) that is consider part of the IL instruction. The IP doesn't need to be adjusted - // in these cases. - bool bAdjustIPForLineNumber = frameNumber == 0 && internalFrames > 0; + // #3 and #4 are the cases that need IP adjusted back because they point after the call instruction + // and may point to the next (incorrect) IL instruction/source line. We distinguish these from #1 + // or #2 by either being non-leaf node stack frame (#4) or the present of an internal stack frame (#3). + bool bAdjustIPForLineNumber = frameNumber > 0 || internalFrames > 0; frameNumber++; - + + // The unmodified IP is displayed which points after the exception in most cases. This means that the + // printed IP and the printed line number often will not map to one another and this is intentional. out.WriteColumn(1, InstructionPtr(ip)); out.WriteColumn(2, MethodNameFromIP(ip, bSuppressLines, bFull, bFull, bAdjustIPForLineNumber)); From 4ac5d8344772a48e7f78865745c8ee4176edb48a Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 13 Jan 2020 18:04:30 -0800 Subject: [PATCH 3/4] Add line number check testcase --- debuggees.sln | 43 ++++++++++++++ .../Debuggees/LineNums/LineNums.csproj | 8 +++ .../Debuggees/LineNums/Program.cs | 29 ++++++++++ src/SOS/SOS.UnitTests/SOS.cs | 6 ++ src/SOS/SOS.UnitTests/Scripts/LineNums.script | 58 +++++++++++++++++++ 5 files changed, 144 insertions(+) create mode 100644 src/SOS/SOS.UnitTests/Debuggees/LineNums/LineNums.csproj create mode 100644 src/SOS/SOS.UnitTests/Debuggees/LineNums/Program.cs create mode 100644 src/SOS/SOS.UnitTests/Scripts/LineNums.script diff --git a/debuggees.sln b/debuggees.sln index 74b27053cf..abffc6dacd 100644 --- a/debuggees.sln +++ b/debuggees.sln @@ -35,6 +35,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SymbolTestApp", "src\SOS\SO EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SymbolTestDll", "src\SOS\SOS.UnitTests\Debuggees\SymbolTestApp\SymbolTestDll\SymbolTestDll.csproj", "{8C27904A-47C0-44C7-B191-88FF34580CBE}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "LineNums", "src\SOS\SOS.UnitTests\Debuggees\LineNums\LineNums.csproj", "{84881FB8-37E1-4D9B-B27E-9831C30DCC04}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Checked|Any CPU = Checked|Any CPU @@ -579,6 +581,46 @@ Global {8C27904A-47C0-44C7-B191-88FF34580CBE}.RelWithDebInfo|x64.Build.0 = Release|Any CPU {8C27904A-47C0-44C7-B191-88FF34580CBE}.RelWithDebInfo|x86.ActiveCfg = Release|Any CPU {8C27904A-47C0-44C7-B191-88FF34580CBE}.RelWithDebInfo|x86.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|Any CPU.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|Any CPU.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|ARM.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|ARM.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|ARM64.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|ARM64.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|x64.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|x64.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|x86.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Checked|x86.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|Any CPU.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|ARM.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|ARM.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|ARM64.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|ARM64.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|x64.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|x64.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|x86.ActiveCfg = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Debug|x86.Build.0 = Debug|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|Any CPU.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|Any CPU.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|ARM.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|ARM.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|ARM64.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|ARM64.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|x64.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|x64.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|x86.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.Release|x86.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|Any CPU.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|Any CPU.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|ARM.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|ARM.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|ARM64.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|ARM64.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|x64.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|x64.Build.0 = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|x86.ActiveCfg = Release|Any CPU + {84881FB8-37E1-4D9B-B27E-9831C30DCC04}.RelWithDebInfo|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -599,6 +641,7 @@ Global {B50D14DB-8EE5-47BD-B412-62FA5C693CC7} = {C3072949-6D24-451B-A308-2F3621F858B0} {112FE2A7-3FD2-4496-8A14-171898AD5CF5} = {C3072949-6D24-451B-A308-2F3621F858B0} {8C27904A-47C0-44C7-B191-88FF34580CBE} = {C3072949-6D24-451B-A308-2F3621F858B0} + {84881FB8-37E1-4D9B-B27E-9831C30DCC04} = {C3072949-6D24-451B-A308-2F3621F858B0} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {46465737-C938-44FC-BE1A-4CE139EBB5E0} diff --git a/src/SOS/SOS.UnitTests/Debuggees/LineNums/LineNums.csproj b/src/SOS/SOS.UnitTests/Debuggees/LineNums/LineNums.csproj new file mode 100644 index 0000000000..a951e54a3a --- /dev/null +++ b/src/SOS/SOS.UnitTests/Debuggees/LineNums/LineNums.csproj @@ -0,0 +1,8 @@ + + + Exe + true + $(BuildProjectFramework) + netcoreapp2.1;netcoreapp3.0 + + diff --git a/src/SOS/SOS.UnitTests/Debuggees/LineNums/Program.cs b/src/SOS/SOS.UnitTests/Debuggees/LineNums/Program.cs new file mode 100644 index 0000000000..721fd9c15f --- /dev/null +++ b/src/SOS/SOS.UnitTests/Debuggees/LineNums/Program.cs @@ -0,0 +1,29 @@ +using System; +using System.Runtime.CompilerServices; + +namespace LineNums +{ + class Program + { + static void Main(string[] args) + { + Foo(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Foo() + { + Bar(); + Bar(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Bar() + { + while (true) + { + throw new Exception("This should be line #25"); + } + } + } +} diff --git a/src/SOS/SOS.UnitTests/SOS.cs b/src/SOS/SOS.UnitTests/SOS.cs index b667cde626..338abcec29 100644 --- a/src/SOS/SOS.UnitTests/SOS.cs +++ b/src/SOS/SOS.UnitTests/SOS.cs @@ -144,6 +144,12 @@ public async Task SimpleThrow(TestConfiguration config) await RunTest(config, "SimpleThrow", "SimpleThrow.script", testTriage: true); } + [SkippableTheory, MemberData(nameof(Configurations))] + public async Task LineNums(TestConfiguration config) + { + await RunTest(config, "LineNums", "LineNums.script", testTriage: true); + } + [SkippableTheory, MemberData(nameof(Configurations))] public async Task NestedExceptionTest(TestConfiguration config) { diff --git a/src/SOS/SOS.UnitTests/Scripts/LineNums.script b/src/SOS/SOS.UnitTests/Scripts/LineNums.script new file mode 100644 index 0000000000..cfbc537359 --- /dev/null +++ b/src/SOS/SOS.UnitTests/Scripts/LineNums.script @@ -0,0 +1,58 @@ +# Test source line numbers for corner cases + +CONTINUE + +LOADSOS + +SOSCOMMAND:PrintException +VERIFY:Exception object:\s+\s+ +VERIFY:Exception type:\s+System\.Exception\s+ +VERIFY:Message:\s+(|This should be line #25)\s+ +VERIFY:InnerException:\s+\s+ +VERIFY:StackTrace\s+\(generated\):\s+ +VERIFY:\s+SP\s+IP\s+Function\s+ +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Bar.*\+0x\s+ +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Foo.*\+0x\s+ +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Main.*\+0x\s+ +VERIFY:HResult:\s+80131500 + +SOSCOMMAND:PrintException -lines +VERIFY:Exception object:\s+\s+ +VERIFY:Exception type:\s+System\.Exception\s+ +VERIFY:Message:\s+(|This should be line #25)\s+ +VERIFY:InnerException:\s+\s+ +VERIFY:StackTrace\s+\(generated\):\s+ +VERIFY:\s+SP\s+IP\s+Function\s+ +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Bar.*\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 25\s*\]\s* +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Foo.*\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 16\s*\]\s* +VERIFY:\s+\s+\s+LineNums.*!LineNums\.Program\.Main.*\+0x\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 10\s*\]\s* + +# Verify that Threads (clrthreads) works +IFDEF:DOTNETDUMP +SOSCOMMAND:clrthreads +ENDIF:DOTNETDUMP +!IFDEF:DOTNETDUMP +SOSCOMMAND:Threads +ENDIF:DOTNETDUMP +VERIFY:\s*ThreadCount:\s+\s+ +VERIFY:\s+UnstartedThread:\s+\s+ +VERIFY:\s+BackgroundThread:\s+\s+ +VERIFY:\s+PendingThread:\s+\s+ +VERIFY:\s+DeadThread:\s+\s+ +VERIFY:\s+Hosted Runtime:\s+no\s+ +VERIFY:\s+ID\s+OSID\s+ThreadOBJ\s+State.*\s+ +VERIFY:\s+\s+\s+\s+.*\s+ + +# Verify that ClrStack with no options works +SOSCOMMAND:ClrStack +VERIFY:.*OS Thread Id:\s+0x\s+.* +VERIFY:\s+Child\s+SP\s+IP\s+Call Site\s+ +VERIFY:\s+\s+\s+LineNums\.Program\.Bar\(\).*\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 25\s*\]\s* +VERIFY:\s+\s+\s+LineNums\.Program\.Foo\(\).*\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 16\s*\]\s* +VERIFY:\s+\s+\s+LineNums\.Program\.Main\(System.String\[\]\).*\s+ +VERIFY:\[.*[\\|/]Debuggees[\\|/]LineNums[\\|/]Program\.cs @ 10\s*\]\s* From 993d8c241f54ec6436c31cabd072937b7296b651 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 13 Jan 2020 22:18:52 -0800 Subject: [PATCH 4/4] Fix non-leaf frames for !pe --- src/SOS/Strike/strike.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/SOS/Strike/strike.cpp b/src/SOS/Strike/strike.cpp index 197a167d1e..4efa101bbc 100644 --- a/src/SOS/Strike/strike.cpp +++ b/src/SOS/Strike/strike.cpp @@ -2478,7 +2478,8 @@ size_t FormatGeneratedException (DWORD_PTR dataPtr, // // #2 and #3 are the cases that need to adjust IP because they point after the call instruction // and may point to the next (incorrect) IL instruction/source line. We distinguish these from - // #1 by the bAsync flag which is set to true for hardware exceptions. + // #1 by the bAsync flag which is set to true for hardware exceptions and that it is a leaf node + // (i == 0). // // When the IP needs to be adjusted it is a lot simpler to decrement IP instead of trying to figure // out the beginning of the instruction. It is enough for GetLineByOffset to return the correct line number. @@ -2486,7 +2487,7 @@ size_t FormatGeneratedException (DWORD_PTR dataPtr, // The unmodified IP is displayed (above by DumpMDInfoBuffer) which points after the exception in most // cases. This means that the printed IP and the printed line number often will not map to one another // and this is intentional. - SUCCEEDED(GetLineByOffset(TO_CDADDR(bAsync ? ste.ip : ste.ip - g_targetMachine->StackWalkIPAdjustOffset()), &linenum, filename, _countof(filename)))) + SUCCEEDED(GetLineByOffset(TO_CDADDR(bAsync && i == 0 ? ste.ip : ste.ip - g_targetMachine->StackWalkIPAdjustOffset()), &linenum, filename, _countof(filename)))) { swprintf_s(wszLineBuffer, _countof(wszLineBuffer), W(" %s [%s @ %d]\n"), so.String(), filename, linenum); }