[SOS][Linux] Support of reading local variables from portable PDB #5897
Conversation
…stem.Diagnostics.Debug.SymbolReader.dll
| 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]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@mikem8361 |
|
LGTM |
| Status = CreateDelegate(hostHandle, domainId, SymbolReaderDllName, | ||
| SymbolReaderClassName, "GetLocalVariableName", | ||
| (void **)&getLocalVariableNameDelegate); | ||
| IfFailRet(Status); |
There was a problem hiding this comment.
This isn't needed, since you're returning Status next anyway.
There was a problem hiding this comment.
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());
|
@adityamandaleeka, thanks for review! |
|
@dotnet-bot Some of these jobs are failing with ./tests/scripts/arm32_ci_script.sh: not found. cc: @prajwal-aithal |
|
@adityamandaleeka The ARM CI check was included yesterday. This error has been seen in PRs that were made (or rebased on earlier masters) before the check was introduced. Other PRs initiated after the CI check was introduced work fine. There must be an issue with pulling the latest master and merging the current PR into that version when the CI begins the check (this is just a hypothesis). A quick fix is to rebase over the latest master and force push to the branch again. I will look into why exactly this error is happening and fix it soon. cc @jashook |
|
@adityamandaleeka this is a CI issue caused from merging the wrong hash. @mmitche can explain better. The arm legs are not the problem, you can ignore the errors. Also @prajwal-aithal you can safely ignore these issues, they are unrelated to your changes. |
|
Thanks @jashook. I'll merge this PR. |
|
@adityamandaleeka @mikem8361 Could you merge PR dotnet/corefx#9571 too? My PR using updated System.Diagnostics.Debug.SymbolReader.dll for reading locals. |
|
@adityamandaleeka @mikem8361 dotnet/dotnet-ci#339 That has a workaround. I hope to roll out the fix this weekend. |
…tnet/coreclr#5897) * Initial support of reading local variables from portable pdb using System.Diagnostics.Debug.SymbolReader.dll * Use SysAllocString and SysFreeString for memory management * Fix coding style after review Commit migrated from dotnet/coreclr@b5ab114
This pull request implements initial support of reading local variables from portable PDB for
clrstack -icommand. To read variables we needSystem.Diagnostics.Debug.SymbolReader.dllwith methodGetLocalVariableNameform the issue: https://github.com/dotnet/corefx/issues/9570.Please pay in attention on FIXME src/ToolBox/SOS/Strike/util.cpp:6386: if we trying to release memory, then we get
SIGABRT("free(): invalid pointer"). @mikem8361, @janvorli could you advise the best way to release memory from managed code?Related issue: https://github.com/dotnet/coreclr/issues/5809
CC @Dmitri-Botcharnikov @seanshpark @chunseoklee