Hosting SOS under ClrMd for ELF dumps. - #124

Merged
mikem8361 merged 6 commits into
dotnet:masterfrom
mikem8361:soshost
Feb 22, 2019
Merged

Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom
mikem8361:soshost

Conversation

@mikem8361

@mikem8361mikem8361 commented Feb 17, 2019

Copy link
Copy Markdown
Contributor

Add interactive dump "analyze" dump support to dotnet-dump project.

Use the System.CommandLine CommandProcessor for the new commands.

Add "sos", "exit", "help", native "modules" and "setthread" commands.

Hosting SOS under ClrMd for ELF dumps.

Add Microsoft.Diagnostic.Utilities containing console, command and help functions.

Strike.cpp/util.cpp clean and fixes.

Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.

DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.

Cleanup some spurious error messages in stack trace commands.

Cleanup ILLDBServices2 interface before it gets shipped.

Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.

Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.

Upgrade to clrmd 1.0.3.

Change SOS.NETCore to the "netstandard2.0" framework.

Mike McLaughlin added 3 commits February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361 added this to the v3.0 milestone Feb 17, 2019
@mikem8361mikem8361 self-assigned this Feb 17, 2019
@mikem8361mikem8361 changed the title SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361

mikem8361 commented Feb 17, 2019

Copy link
Copy Markdown
ContributorAuthor

@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.

Thanks.

jonsequitur
jonsequitur reviewed Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs Outdated
@jonsequitur

jonsequitur commented Feb 18, 2019

Copy link
Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.

@mikem8361

mikem8361 commented Feb 18, 2019

Copy link
Copy Markdown
ContributorAuthor

When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).

noahfalk
noahfalk reviewed Feb 21, 2019
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.

One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.

Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs Outdated
Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs Outdated
Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs Outdated
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs Outdated
Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs Outdated
Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs Outdated
Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs Outdated
Comment threadsrc/Tools/dotnet-dump/Program.cs Outdated
@mikem8361

mikem8361 commented Feb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.

@noahfalk

noahfalk commented Feb 22, 2019

Copy link
Copy Markdown
Member

How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.

@mikem8361

mikem8361 commented Feb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.

Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.

If we have time to both, that would be even better.

@mikem8361

mikem8361 commented Feb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.

@mikem8361

mikem8361 commented Feb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?

noahfalk
noahfalk approved these changes Feb 22, 2019
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Go for it.

One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.

Mike McLaughlin added 3 commits February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361 merged commit e432864 into dotnet:masterFeb 22, 2019
@mikem8361
mikem8361 deleted the soshost branch February 22, 2019 23:13
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes
+1 more reviewer
@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels

None yet

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Hosting SOS under ClrMd for ELF dumps. - #124
Merged
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Feb 22, 2019
Merged
Hosting SOS under ClrMd for ELF dumps.#124
mikem8361 merged 6 commits into
dotnet:masterfrom 
mikem8361:soshost
Conversation
@mikem8361
@mikem8361mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

Contributor
Add interactive dump "analyze" dump support to dotnet-dump project.
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
Hosting SOS under ClrMd for ELF dumps.
Add Microsoft.Diagnostic.Utilities containing console, command and help functions.
Strike.cpp/util.cpp clean and fixes.
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
Cleanup ILLDBServices2 interface before it gets shipped.
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Upgrade to clrmd 1.0.3.
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
added 3 commits
February 17, 2019 11:49
Mike McLaughlin
Upgrade to clrmd 1.0.3.
d42c212
Change SOS.NETCore to the "netstandard2.0" framework.
Mike McLaughlin
Cleanup ILLDBServices2 interface.
754619e
Add runtineOnly option to LoadNativeSymbols to use to get DAC/DBI module name.
Use LoadNativeSymbols(true) to get the DAC/DBI modules when don't exist locally.
Mike McLaughlin
Strike.cpp/util.cpp clean and fixes.
61efb48
Cleanup GetCoreClrDirectory service function. No need to have separate GetModuleDirectory method.
DacpGetModuleData.Request fails on some modules. Skip them in GetModuleFromAddress.
Cleanup some spurious error messages in stack trace commands.
@mikem8361mikem8361
added this to the v3.0 milestone
Feb 17, 2019
@mikem8361mikem8361
self-assigned this
Feb 17, 2019
@mikem8361mikem8361
changed the title
SoshostHosting SOS under ClrMd for ELF dumps.Feb 17, 2019
@mikem8361
mikem8361
commented
Feb 17, 2019

Copy link


Copy Markdown

ContributorAuthor
@jonsequitur if you have time, could you take a quick look how I used System.CommandLine in the "Add Microsoft.Diagnostic.Utilities containing console, command and help functions." commit.
Thanks.
jonsequitur
jonsequitur
reviewed
Feb 17, 2019
View reviewed changes
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

@jonsequitur
jonsequitur
commented
Feb 18, 2019

Copy link


Copy Markdown

A good deal of the usage of System.CommandLine here covers territory that either I'm working to make easier with my binding PR (dotnet/command-line-api#408) or that Kathleen is working to make easier with her app model work (currently no open PR). We should chat this week.
@mikem8361
mikem8361
commented
Feb 18, 2019

Copy link


Copy Markdown

ContributorAuthor
When your and Kathleen's work is finished and in, I'll look at using at some point. We don't want to wait or depend on this future work (just for now).
noahfalk
noahfalk
reviewed
Feb 21, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

A few odds and ends I noticed, but mostly looks fine.
One big open question, what is the plan for testing? I don't think we need to re-test the details of SOS commands that are tested elsewhere, but there are thousands of new lines of code showing up and no corresponding testing so far. That makes me nervous.
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/CommandProcessor.cs
Outdated

Comment threadsrc/Microsoft.Diagnostic.Utilities/AssemblyResolver.cs
Comment threadsrc/Microsoft.Diagnostic.Utilities/Command/Attributes.cs
Outdated

Comment threadsrc/SOS/SOS.Hosting/LLDBServicesWrapper.cs
Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Analyzer.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Commands/SOSCommand.cs
Outdated

Comment threadsrc/Tools/dotnet-dump/Program.cs
Outdated

@mikem8361
mikem8361
commented
Feb 21, 2019

Copy link


Copy Markdown

ContributorAuthor
For testing, I plan to modify the SOS runner to run dotnet-debug analyze with the scripts/core dumps that make sense. This isn't to test the native SOS as much as to test the SOS hosting infrastructure including the keyboard/console support.
@noahfalk
noahfalk
commented
Feb 22, 2019

Copy link


Copy Markdown

Member
How would you feel about shifting some of the emphasis from scenario tests to unit tests, and putting those unit tests in this repo that will run as part of the CI? The old SOS tests were awkward because it was difficult to run anything without being hosted in a debugger, and then it was difficult to interact with SOS other than through the command-line interface. But now that we own all the layers and it is primarily C# libraries, I think we could align with standard conventions much more closely.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
I do like the idea of unit tests without having to deal with a debugger like lldb or cdb. That is one reason I tried to put the actual SOS hosting functionality in a separate assembly (same with the SOS.Installer). Writing a xunit test directly without having the input script, etc would make it easier to test even more of the SOS commands.
Given all that plumbing the dotnet-dump analyze repl into the current SOS runner would exercise the infrastructure better (the interop layer, command processor and even the console provider a little). I don't think it would take that much time to change the SOS runner.
If we have time to both, that would be even better.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Actually now that I think about this more, testing the native SOS commands through the SOSHost directly in a xunit test would exercise most of the infrastructure and the native SOS command itself. Using the SOS runner probably wouldn't give us that much coverage of the command processor/repl code.
@mikem8361
mikem8361
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
@noahfalk Thanks for all the feedback. I really appreciate the effort it takes for these reviews. I filed some follow up issues. Do you think this PR is ready to merge?
noahfalk
noahfalk
approved these changes
Feb 22, 2019
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Go for it.
One suggestion either in this PR or a near future one. If that assembly resolver is only supposed to deal with SOS.NetCore.dll, I would add comments to that effect and ideally add checks in the code to ensure that you don't inadvertently start affecting the resolutions of other assemblies.
Mike McLaughlin
added 3 commits
February 22, 2019 15:03
Mike McLaughlin
Add Microsoft.Diagnostic.Repl containing console and command functions.
551fdcc
Mike McLaughlin
Hosting SOS under ClrMd for ELF dumps.
4cb670b
Mike McLaughlin
Add interactive dump "analyze" dump support to dotnet-dump project.
6d59be7
Use the System.CommandLine CommandProcessor for the new commands.
Add "sos", "exit", "help", native "modules" and "setthread" commands.
@mikem8361
mikem8361
merged commit e432864
into
dotnet:masterFeb 22, 2019
@mikem8361
mikem8361
deleted the

soshost

branch
February 22, 2019 23:13
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 20, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@jonsequiturjonsequiturjonsequitur left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@mikem8361mikem8361

Labels


None yet


Projects


None yet


Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mikem8361@jonsequitur@noahfalk