Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[SOS][Linux] Support of reading local variables from portable PDB #5897

Merged
merged 3 commits into from
Jun 24, 2016
Merged

[SOS][Linux] Support of reading local variables from portable PDB #5897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a908de0
Initial support of reading local variables from portable pdb using Sy…
Jun 17, 2016
1ec4a4c
Use SysAllocString and SysFreeString for memory management
Jun 22, 2016
2ded26c
Fix coding style after review
Jun 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/ToolBox/SOS/Strike/strike.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11562,26 +11562,22 @@ class ClrStackImplWithICorDebug
IfFailRet(pLocalsEnum->GetCount(&cLocals));
if (cLocals > 0 && bLocals)
{
#ifndef FEATURE_PAL
bool symbolsAvailable = false;
SymbolReader symReader;
if(SUCCEEDED(symReader.LoadSymbols(pMD, pModule)))
symbolsAvailable = true;
#endif
ExtOut("\nLOCALS:\n");
for (ULONG i=0; i < cLocals; i++)
{
ULONG paramNameLen = 0;
WCHAR paramName[mdNameLen] = W("\0");

ToRelease<ICorDebugValue> pValue;
#ifndef FEATURE_PAL
if(symbolsAvailable)
{
Status = symReader.GetNamedLocalVariable(pILFrame, i, paramName, mdNameLen, &pValue);
}
else
#endif
{
ULONG cArgsFetched;
Status = pLocalsEnum->Next(1, &pValue, &cArgsFetched);
Expand Down
44 changes: 36 additions & 8 deletions src/ToolBox/SOS/Strike/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ PIMAGEHLP_SYMBOL sym = (PIMAGEHLP_SYMBOL) symBuffer;
void *SymbolReader::coreclrLib;
ResolveSequencePointDelegate SymbolReader::resolveSequencePointDelegate;
LoadSymbolsForModuleDelegate SymbolReader::loadSymbolsForModuleDelegate;
GetLocalVariableName SymbolReader::getLocalVariableNameDelegate;
#endif // !FEATURE_PAL

const char * const CorElementTypeName[ELEMENT_TYPE_MAX]=
Expand Down Expand Up @@ -6284,6 +6285,11 @@ HRESULT SymbolReader::LoadCoreCLR() {
SymbolReaderClassName, "LoadSymbolsForModule",
(void **)&loadSymbolsForModuleDelegate);
IfFailRet(Status);
Status = CreateDelegate(hostHandle, domainId, SymbolReaderDllName,
SymbolReaderClassName, "GetLocalVariableName",
(void **)&getLocalVariableNameDelegate);
IfFailRet(Status);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed, since you're returning Status next anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, it looks like in this change and in some previous changes, the following pattern has been added:

Status = Something();
IfFailRet(Status);

whereas the convention is normally:

IfFailRet(Something());


return Status;
}
#endif //FEATURE_PAL
Expand Down Expand Up @@ -6363,30 +6369,50 @@ HRESULT SymbolReader::LoadSymbols(IMetaDataImport * pMD, ULONG64 baseAddress, __
if (Status != S_OK)
return Status;

char szName[mdNameLen];
WideCharToMultiByte(CP_ACP, 0, pModuleName, (int) (_wcslen(pModuleName) + 1),
szName, mdNameLen, NULL, NULL);
return !loadSymbolsForModuleDelegate(szName);
m_szModuleName, mdNameLen, NULL, NULL);
return !loadSymbolsForModuleDelegate(m_szModuleName);
#endif // FEATURE_PAL
}

HRESULT SymbolReader::GetNamedLocalVariable(ISymUnmanagedScope * pScope, ICorDebugILFrame * pILFrame, mdMethodDef methodToken, ULONG localIndex, __inout_ecount(paramNameLen) WCHAR* paramName, ULONG paramNameLen, ICorDebugValue** ppValue)
{
HRESULT Status = S_OK;
#ifdef FEATURE_PAL
if (getLocalVariableNameDelegate == nullptr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style comment: I noticed that there are a few different styles all in use in this area. Sometimes there are braces around single-statement 'if' blocks, and sometimes there aren't. Brace position should also be consistent with the surrounding code (I think before your changes, most/all of the file used Allman style).

Status = LoadCoreCLR();
if (Status != S_OK)
return Status;
// FIXME: we need to find a way to release memory after getting string from managed code.
WCHAR *wszParamName = new (std::nothrow) WCHAR[mdNameLen];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The managed code shouldn't free this memory; it should be freed when the function returns. The managed code shouldn't save this pointer in any global state. If it is, then maybe the managed code needs to change.

If you absolutely have to save the string in the managed code, then you could allocate is a bstr with SysAllocString and use Marshal.FreeBSTR to free it managed code. But I don't remember if this is supported in the xplat platforms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like @Dmitri-Botcharnikov is changing GetLocalVariableName to return a BSTR for the name so my comment is moot other than using SysFreeString to free it in this native code. I don't remember if we properly support BSTRs on the xplat platforms. It still may be simpler to allocate a buffer like the current code and free it in native code. The managed code just copies the name string to the incoming buffer.

if (wszParamName == NULL) {
return E_OUTOFMEMORY;
}
int ret = getLocalVariableNameDelegate(m_szModuleName, methodToken,
localIndex, &wszParamName);
if (ret) {
wcscpy_s(paramName, _wcslen(wszParamName) + 1, wszParamName);
paramNameLen = _wcslen(paramName);

if (SUCCEEDED(pILFrame->GetLocalVariable(localIndex, ppValue)) && (*ppValue != NULL)) {
return S_OK;
} else {
*ppValue = NULL;
return E_FAIL;
}
}
return E_FAIL;

#else
if(pScope == NULL)
{
#ifndef FEATURE_PAL
ToRelease<ISymUnmanagedMethod> pSymMethod;
IfFailRet(m_pSymReader->GetMethod(methodToken, &pSymMethod));

ToRelease<ISymUnmanagedScope> pScope;
IfFailRet(pSymMethod->GetRootScope(&pScope));

return GetNamedLocalVariable(pScope, pILFrame, methodToken, localIndex, paramName, paramNameLen, ppValue);
#else
return E_FAIL;
#endif // FEATURE_PAL
}
else
{
Expand All @@ -6404,7 +6430,7 @@ HRESULT SymbolReader::GetNamedLocalVariable(ISymUnmanagedScope * pScope, ICorDeb
if(varIndexInMethod != localIndex)
continue;

ULONG32 nameLen = 0;
ULONG32 nameLen = 0;
if(FAILED(pLocals[i]->GetName(paramNameLen, &nameLen, paramName)))
swprintf_s(paramName, paramNameLen, W("local_%d\0"), localIndex);

Expand Down Expand Up @@ -6440,7 +6466,9 @@ HRESULT SymbolReader::GetNamedLocalVariable(ISymUnmanagedScope * pScope, ICorDeb
for(ULONG j = 0; j < numChildren; j++) pChildren[j]->Release();

}

return E_FAIL;
#endif // FEATURE_PAL
}

HRESULT SymbolReader::GetNamedLocalVariable(ICorDebugFrame * pFrame, ULONG localIndex, __inout_ecount(paramNameLen) WCHAR* paramName, ULONG paramNameLen, ICorDebugValue** ppValue)
Expand Down
3 changes: 3 additions & 0 deletions src/ToolBox/SOS/Strike/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2358,6 +2358,7 @@ class PERvaMemoryReader : IDiaReadExeAtRVACallback
#ifdef FEATURE_PAL
typedef int (*ResolveSequencePointDelegate)(const char*, const char*, unsigned int, unsigned int*, unsigned int*);
typedef int (*LoadSymbolsForModuleDelegate)(const char*);
typedef int (*GetLocalVariableName)(const char*, int, int, WCHAR**);
static const char *SymbolReaderDllName = "System.Diagnostics.Debug.SymbolReader";
static const char *SymbolReaderClassName = "System.Diagnostics.Debug.SymbolReader.SymbolReader";
#endif //FEATURE_PAL
Expand All @@ -2368,8 +2369,10 @@ class SymbolReader
ISymUnmanagedReader* m_pSymReader;
#ifdef FEATURE_PAL
static void *coreclrLib;
char m_szModuleName[mdNameLen];
static ResolveSequencePointDelegate resolveSequencePointDelegate;
static LoadSymbolsForModuleDelegate loadSymbolsForModuleDelegate;
static GetLocalVariableName getLocalVariableNameDelegate;
#endif

private:
Expand Down