Implement CLRMA interfaces on top of DAC APIs #4667
Conversation
mikem8361
force-pushed
the
clrma
branch
2 times, most recently
from
25990e7 to
aa628f0
Compare
|
@dotnet/dotnet-diag anybody up for a code review? |
tommcdon
left a comment
tommcdon
left a comment
There was a problem hiding this comment.
I left some comments for your review.
Add "clrmaconfig" command to control clrma logging, direct DAC and managed support. Move SOSExtensions to separate cpp/h files. Cleanup Extensions::FlushCheck. Needed to call in the clrma code. INIT_API_EXT macro cleanup: added ExtInit function that does all command entry init. Change the no managed hosting fallback code (platform/*) to use the IDebuggerServices instance instead of the global dbgeng variables because it is always available/setup. This allows the CLRMA support to work with no managed code.
…::StackTraceEnabled flag; no longer needed.
| using System.Text; | ||
|
|
||
| namespace Microsoft.Diagnostics.TestHelpers | ||
| namespace Microsoft.Diagnostics.DebugServices.Implementation |
There was a problem hiding this comment.
This was OK as test code, but I am not sure it properly handles multibyte sequences. Why is this needed? And why not just take line input as it and copy as-is? What breaks with line feeds?
There was a problem hiding this comment.
This has always been used in the REPL console service. It deals with the .Write() of partial lines and turns them into full lines. It has been part of SOS for a long time; I used in the Midori debugger. When we have problems with multibyte UTF sequences, I'll fix it.
There was a problem hiding this comment.
I don't like that it has been duplicated twice (once in REPL and this new one in the impl assembly) but we don't have a common utility assembly yet for things like this.
| VTable.FlushCheck(Self); | ||
| } | ||
|
|
||
| public ImmutableArray<string> ExecuteHostCommand(string commandLine, DEBUG_OUTPUT interestMask = DEBUG_OUTPUT.NORMAL) |
There was a problem hiding this comment.
Any reason we are opting for ImmutableArray here over say IReadOnlyList? Is there anything that depends on CoW semmantics?
There was a problem hiding this comment.
What CoW (I assume you mean copy-on-write) semantics does ImmutableArray have? I just want an array that the elements can't be changed/written. Why would IReadOnlyList be better?
There was a problem hiding this comment.
I think I see now. List already implements IReadOnlyList and no copying/conversion would have to happen.
There was a problem hiding this comment.
I'll change it to IReadOnlyList.
There was a problem hiding this comment.
ImmutableArray allows you to write - and it just allocates a new array. It just doesn't modify the old one. Things like https://learn.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutablearray-1.add?view=net-8.0. So if I don't expect that to be a concern - I just prefer IReadOnly...
There was a problem hiding this comment.
ImmutableArrays have lower overhead than IReadOnlyList, or other similar constructs.
IMO though, it actually wasn't worthwhile to use ImmutableArray in ClrMD. The dependency alone is a headache to maintain (as you see from our conversation about 6.0 vs 8.0), the perf benefit isn't noticeable (especially in something as slow as debugging), and the contract guarantees of immutability don't make things easier. In retrospect, just returning ClrModule[] instead of ImmutableArray<ClrModule> would have been fine. But that ship has sailed for ClrMD to make that big of a change without a lot of churn.
Avoiding immutable is a good decision. Even just returning raw arrays is conditionally fine, if it doesn't hurt anything if a user decides to modify one. Just my 2 cents.
5 participants
Add "clrmaconfig" command to control clrma logging, direct DAC and managed support.
Move SOSExtensions to separate cpp/h files.
Cleanup Extensions::FlushCheck. Needed to call in the clrma code.
INIT_API_EXT macro cleanup: added ExtInit function that does all command entry init.
Change the no managed hosting fallback code (platform/*) to use the IDebuggerServices instance instead of the global dbgeng variables because it is always available/setup. This allows the CLRMA support to work with no managed code.