Skip to content

Commit

Permalink
Fix single-file unhandled exception source/line info (dotnet#52725)
Browse files Browse the repository at this point in the history 
Issues:
    dotnet#51461
    dotnet#11261
    dotnet#18423
  • Loading branch information
@mikem8361
mikem8361 authored May 14, 2021
1 parent 7185031 commit 83f64e5
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 21 deletions.
8 changes: 5 additions & 3 deletions src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackFrameHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ internal sealed class StackFrameHelper
private Assembly?[]? rgAssembly;
private IntPtr[]? rgLoadedPeAddress;
private int[]? rgiLoadedPeSize;
private bool[]? rgiIsFileLayout;
private IntPtr[]? rgInMemoryPdbAddress;
private int[]? rgiInMemoryPdbSize;
// if rgiMethodToken[i] == 0, then don't attempt to get the portable PDB source/info
Expand All @@ -39,7 +40,7 @@ internal sealed class StackFrameHelper
#pragma warning restore 414

private delegate void GetSourceLineInfoDelegate(Assembly? assembly, string assemblyPath, IntPtr loadedPeAddress,
int loadedPeSize, IntPtr inMemoryPdbAddress, int inMemoryPdbSize, int methodToken, int ilOffset,
int loadedPeSize, bool isFileLayout, IntPtr inMemoryPdbAddress, int inMemoryPdbSize, int methodToken, int ilOffset,
out string? sourceFile, out int sourceLine, out int sourceColumn);

private static GetSourceLineInfoDelegate? s_getSourceLineInfo;
Expand All @@ -58,6 +59,7 @@ public StackFrameHelper(Thread? target)
rgAssembly = null;
rgLoadedPeAddress = null;
rgiLoadedPeSize = null;
rgiIsFileLayout = null;
rgInMemoryPdbAddress = null;
rgiInMemoryPdbSize = null;
dynamicMethods = null;
Expand Down Expand Up @@ -110,7 +112,7 @@ internal void InitializeSourceInfo(int iSkip, bool fNeedFileInfo, Exception? exc

Type[] parameterTypes = new Type[]
{
typeof(Assembly), typeof(string), typeof(IntPtr), typeof(int), typeof(IntPtr),
typeof(Assembly), typeof(string), typeof(IntPtr), typeof(int), typeof(bool), typeof(IntPtr),
typeof(int), typeof(int), typeof(int),
typeof(string).MakeByRefType(), typeof(int).MakeByRefType(), typeof(int).MakeByRefType()
};
Expand All @@ -137,7 +139,7 @@ internal void InitializeSourceInfo(int iSkip, bool fNeedFileInfo, Exception? exc
// ENC or the source/line info was already retrieved, the method token is 0.
if (rgiMethodToken![index] != 0)
{
s_getSourceLineInfo!(rgAssembly![index], rgAssemblyPath![index]!, rgLoadedPeAddress![index], rgiLoadedPeSize![index],
s_getSourceLineInfo!(rgAssembly![index], rgAssemblyPath![index]!, rgLoadedPeAddress![index], rgiLoadedPeSize![index], rgiIsFileLayout![index],
rgInMemoryPdbAddress![index], rgiInMemoryPdbSize![index], rgiMethodToken![index],
rgiILOffset![index], out rgFilename![index], out rgiLineNumber![index], out rgiColumnNumber![index]);
}
Expand Down
14 changes: 13 additions & 1 deletion src/coreclr/vm/debugdebugger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ FCIMPL4(void, DebugStackTrace::GetStackFramesInternal,
OBJECTREF loadedPeSizeArray = AllocatePrimitiveArray(ELEMENT_TYPE_I4, data.cElements);
SetObjectReference( (OBJECTREF *)&(pStackFrameHelper->rgiLoadedPeSize), (OBJECTREF)loadedPeSizeArray);

// Allocate memory for the IsFileLayout flags
OBJECTREF isFileLayouts = AllocatePrimitiveArray(ELEMENT_TYPE_BOOLEAN, data.cElements);
SetObjectReference( (OBJECTREF *)&(pStackFrameHelper->rgiIsFileLayout), (OBJECTREF)isFileLayouts);

// Allocate memory for the InMemoryPdbAddress
BASEARRAYREF inMemoryPdbAddressArray = (BASEARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_I, data.cElements);
SetObjectReference( (OBJECTREF *)&(pStackFrameHelper->rgInMemoryPdbAddress), (OBJECTREF)inMemoryPdbAddressArray);
Expand Down Expand Up @@ -722,6 +726,14 @@ FCIMPL4(void, DebugStackTrace::GetStackFramesInternal,
I4 *pLoadedPeSize = (I4 *)((I4ARRAYREF)pStackFrameHelper->rgiLoadedPeSize)->GetDirectPointerToNonObjectElements();
pLoadedPeSize[iNumValidFrames] = (I4)peSize;

// Set flag indicating PE file in memory has the on disk layout
if (!pPEFile->IsDynamic())
{
// This flag is only available for non-dynamic assemblies.
U1 *pIsFileLayout = (U1 *)((BOOLARRAYREF)pStackFrameHelper->rgiIsFileLayout)->GetDirectPointerToNonObjectElements();
pIsFileLayout[iNumValidFrames] = (U1) pPEFile->GetLoaded()->IsFlat();
}

// If there is a in memory symbol stream
CGrowableStream* stream = pModule->GetInMemorySymbolStream();
if (stream != NULL)
Expand All @@ -738,7 +750,7 @@ FCIMPL4(void, DebugStackTrace::GetStackFramesInternal,
else
{
// Set the pdb path (assembly file name)
const SString& assemblyPath = pPEFile->GetPath();
SString assemblyPath = pPEFile->GetIdentityPath();
if (!assemblyPath.IsEmpty())
{
OBJECTREF obj = (OBJECTREF)StringObject::NewString(assemblyPath);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/debugdebugger.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class StackFrameHelper : public Object
PTRARRAYREF rgAssembly;
BASEARRAYREF rgLoadedPeAddress;
I4ARRAYREF rgiLoadedPeSize;
BOOLARRAYREF rgiIsFileLayout;
BASEARRAYREF rgInMemoryPdbAddress;
I4ARRAYREF rgiInMemoryPdbSize;
// if rgiMethodToken[i] == 0, then don't attempt to get the portable PDB source/info
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/pefile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ class PEFile
// ------------------------------------------------------------

// Path is the file path to the file; empty if not a file
const SString &GetPath();
const SString& GetPath();
const SString& GetIdentityPath();

#ifdef DACCESS_COMPILE
// This is the metadata module name. Used as a hint as file name.
Expand Down
28 changes: 24 additions & 4 deletions src/coreclr/vm/pefile.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ inline void PEFile::GetMVID(GUID *pMvid)
// Descriptive strings
// ------------------------------------------------------------

inline const SString &PEFile::GetPath()
inline const SString& PEFile::GetPath()
{
CONTRACTL
{
Expand All @@ -184,10 +184,31 @@ inline const SString &PEFile::GetPath()
{
return SString::Empty();
}
else
return m_identity->GetPath();
return m_identity->GetPath();
}

//
// Returns the identity path even for single-file/bundled apps.
//
inline const SString& PEFile::GetIdentityPath()
{
CONTRACTL
{
INSTANCE_CHECK;
GC_NOTRIGGER;
NOTHROW;
CANNOT_TAKE_LOCK;
MODE_ANY;
SUPPORTS_DAC;
}
CONTRACTL_END;

if (m_identity == nullptr)
{
return SString::Empty();
}
return m_identity->GetPath();
}

#ifdef DACCESS_COMPILE
inline const SString &PEFile::GetModuleFileNameHint()
Expand Down Expand Up @@ -306,7 +327,6 @@ inline PEAssembly *PEFile::GetAssembly() const
WRAPPER_NO_CONTRACT;
_ASSERTE(IsAssembly());
return dac_cast<PTR_PEAssembly>(this);

}

// ------------------------------------------------------------
Expand Down
25 changes: 13 additions & 12 deletions src/libraries/System.Diagnostics.StackTrace/src/System/Diagnostics/StackTraceSymbols.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,23 @@ void IDisposable.Dispose()
/// <param name="assemblyPath">file path of the assembly or null</param>
/// <param name="loadedPeAddress">loaded PE image address or zero</param>
/// <param name="loadedPeSize">loaded PE image size</param>
/// <param name="isFileLayout">if true, the PE image is file layout, false it is loaded layout</param>
/// <param name="inMemoryPdbAddress">in memory PDB address or zero</param>
/// <param name="inMemoryPdbSize">in memory PDB size</param>
/// <param name="methodToken">method token</param>
/// <param name="ilOffset">il offset of the stack frame</param>
/// <param name="sourceFile">source file return</param>
/// <param name="sourceLine">line number return</param>
/// <param name="sourceColumn">column return</param>
internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize,
internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize, bool isFileLayout,
IntPtr inMemoryPdbAddress, int inMemoryPdbSize, int methodToken, int ilOffset,
out string? sourceFile, out int sourceLine, out int sourceColumn)
{
sourceFile = null;
sourceLine = 0;
sourceColumn = 0;

MetadataReader? reader = TryGetReader(assembly, assemblyPath, loadedPeAddress, loadedPeSize, inMemoryPdbAddress, inMemoryPdbSize);
MetadataReader? reader = TryGetReader(assembly, assemblyPath, loadedPeAddress, loadedPeSize, isFileLayout, inMemoryPdbAddress, inMemoryPdbSize);
if (reader != null)
{
Handle handle = MetadataTokens.Handle(methodToken);
Expand Down Expand Up @@ -105,6 +106,7 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
/// (pointed to by <paramref name="inMemoryPdbAddress"/> and <paramref name="inMemoryPdbSize"/>).
/// </param>
/// <param name="loadedPeSize">loaded PE image size</param>
/// <param name="isFileLayout">if true, the PE image is file layout, false it is loaded layout</param>
/// <param name="inMemoryPdbAddress">in memory PDB address or zero</param>
/// <param name="inMemoryPdbSize">in memory PDB size</param>
/// <returns>reader</returns>
Expand All @@ -113,7 +115,7 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
/// underlying ConditionalWeakTable doesn't keep the assembly alive, so cached types will be
/// correctly invalidated when the Assembly is unloaded by the GC.
/// </remarks>
private unsafe MetadataReader? TryGetReader(Assembly assembly, string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize, IntPtr inMemoryPdbAddress, int inMemoryPdbSize)
private unsafe MetadataReader? TryGetReader(Assembly assembly, string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize, bool isFileLayout, IntPtr inMemoryPdbAddress, int inMemoryPdbSize)
{
if ((loadedPeAddress == IntPtr.Zero || assemblyPath == null) && inMemoryPdbAddress == IntPtr.Zero)
{
Expand All @@ -127,7 +129,7 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
{
return (inMemoryPdbAddress != IntPtr.Zero) ?
TryOpenReaderForInMemoryPdb(inMemoryPdbAddress, inMemoryPdbSize) :
TryOpenReaderFromAssemblyFile(assemblyPath!, loadedPeAddress, loadedPeSize);
TryOpenReaderFromAssemblyFile(assemblyPath!, loadedPeAddress, loadedPeSize, isFileLayout);
});

// The reader has already been open, so this doesn't throw.
Expand Down Expand Up @@ -160,13 +162,12 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
}
}

private static unsafe PEReader? TryGetPEReader(string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize)
private static unsafe PEReader? TryGetPEReader(string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize, bool isFileLayout)
{
// TODO: https://github.com/dotnet/runtime/issues/18423
//if (loadedPeAddress != IntPtr.Zero && loadedPeSize > 0)
//{
// return new PEReader((byte*)loadedPeAddress, loadedPeSize, isLoadedImage: true);
//}
if (loadedPeAddress != IntPtr.Zero && loadedPeSize > 0)
{
return new PEReader((byte*)loadedPeAddress, loadedPeSize, isLoadedImage: !isFileLayout);
}

Stream? peStream = TryOpenFile(assemblyPath);
if (peStream != null)
Expand All @@ -177,9 +178,9 @@ internal void GetSourceLineInfo(Assembly assembly, string assemblyPath, IntPtr l
return null;
}

private static MetadataReaderProvider? TryOpenReaderFromAssemblyFile(string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize)
private static MetadataReaderProvider? TryOpenReaderFromAssemblyFile(string assemblyPath, IntPtr loadedPeAddress, int loadedPeSize, bool isFileLayout)
{
using (var peReader = TryGetPEReader(assemblyPath, loadedPeAddress, loadedPeSize))
using (var peReader = TryGetPEReader(assemblyPath, loadedPeAddress, loadedPeSize, isFileLayout))
{
if (peReader == null)
{
Expand Down

0 comments on commit 83f64e5

Please sign in to comment.