changes to allow compatibility with ClrMdV2

[neilmsimmons]

No description provided.

[neilmsimmons]

I've been using this with linqpad, it seems to work but it might still need some tidying up

[JeffCyr]

Hey Neil, sorry for being so late for this review!

I left a lot of comments in case you want to continue on this path. But I'm wondering if ClrMD.Extensions still have a reason to exist with ClrMD v2. In any case, supporting v2 would probably need to be in a side branch, I'm surprised to see that BlockingObject was removed from v2. The v1 wrapper would need to continue on the side to support the missing features and older .NET frameworks no longer supported by v2.

ClrMD.Extensions was built when ClrMD itself was very hard to use, but now it looks like the proper abstractions exist to directly use ClrMD in a productive way.

Maybe it would be best to just tell LINQPad how to Dump() the ClrMD classes (https://www.linqpad.net/CustomizingDump.aspx)

[JeffCyr]

Commented code should be removed

[JeffCyr]

This probably won't be a bottleneck, but Type.AsEnum().EnumerateValues().FirstOrDefault seems inefficient. That said I don't know how Type.GetEnumName used to work, maybe it was already implemented with something simliar.

[JeffCyr]

This will iterate all types to find the declaringType everytime ClrDynamic.FindField is called. This probably introduces a performance bottleneck.

If there isn't a way to lookup by type name, a cached map should be preprocessed to do this only once

[JeffCyr]

Remove commented line

[JeffCyr]

Is this still true in ClrMD v2?

[JeffCyr]

AttachFlag no longer exists? Is there an alternative to attach without freezing the process?

[JeffCyr]

Cleanup commented code

[JeffCyr]

Looks like heap.EnumerateTypes() can be used directly

[JeffCyr]

Missing space after comma

[JeffCyr]

Cleanup commented code

[neilmsimmons]

Hey JF, Hope that you are doing well, for some reason I only just saw this in my inbox whilst I was looking for something else. When I made the changes I was doing it with the mindset of making the bare minimum number of changes to allow our current clrmd.extension scripts to work with the clrmdV2. It is also clear that I'm pretty novice at this stuff (aka I don't know what I'm doing). I will take a look at your suggestions when I get a chance and see what will be the best way forward. Thanks for taking the time to review everything. Neil

…

On Sat, Apr 10, 2021 at 10:40 PM Jeff Cyr ***@***.***> wrote: ***@***.**** requested changes on this pull request. Hey Neil, sorry for being so late for this review! I left a lot of comments in case you want to continue on this path. But I'm wondering if ClrMD.Extensions still have a reason to exist with ClrMD v2. In any case, supporting v2 would probably need to be in a side branch, I'm surprised to see that BlockingObject was removed from v2. The v1 wrapper would need to continue on the side to support the missing features and older .NET frameworks no longer supported by v2. ClrMD.Extensions was built when ClrMD itself was very hard to use, but now it looks like to proper abstraction exists to directly use ClrMD in a productive way. Maybe it would be best to just tell LINQPad how to Dump() the ClrMD classes (https://www.linqpad.net/CustomizingDump.aspx) ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -60,7 +61,8 @@ public class ClrDynamic : DynamicObject, IEnumerable<ClrDynamic>, IComparable, I if (!Type.IsArray) throw new InvalidOperationException(string.Format("Type '{0}' is not an array", Type.Name)); - int arrayLength = Type.GetArrayLength(Address); + //int arrayLength = Type.GetArrayLength(Address); Commented code should be removed ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -60,7 +61,8 @@ public class ClrDynamic : DynamicObject, IEnumerable<ClrDynamic>, IComparable, I if (!Type.IsArray) throw new InvalidOperationException(string.Format("Type '{0}' is not an array", Type.Name)); - int arrayLength = Type.GetArrayLength(Address); + //int arrayLength = Type.GetArrayLength(Address); + int arrayLength = Heap.GetObject(Address).AsArray().Length; Is there a way to get the array length without allocating a new array with AsArray()? The current statement could lead to poor performance. ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -78,7 +80,8 @@ public int ArrayLength if (!Type.IsArray) throw new InvalidOperationException(string.Format("Type '{0}' is not an array", Type.Name)); - return Type.GetArrayLength(Address); + //return Type.GetArrayLength(Address); + return Heap.GetObject(Address).AsArray().Length; ; Same as above ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -91,12 +94,14 @@ public object SimpleDisplayValue get { if (Type.IsEnum) - return Type.GetEnumName(SimpleValue) ?? SimpleValue.ToString(); + { + return Type.AsEnum().EnumerateValues().FirstOrDefault(f => Convert.ToInt64(f.Value) == Convert.ToInt64(SimpleValue)).Name ?? SimpleValue.ToString(); This probably won't be a bottleneck, but Type.AsEnum().EnumerateValues().FirstOrDefault seems inefficient. That said I don't know how Type.GetEnumName used to work, maybe it was already implemented with something simliar. ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -130,8 +135,11 @@ public bool IsUndefined() private static ClrInstanceField FindField(ObfuscatedField oField) { - TypeName delcaringType = ClrMDSession.Current.ObfuscateType(oField.DeclaringType); - var target = ClrMDSession.Current.Heap.GetTypeByName(delcaringType); + TypeName declaringType = ClrMDSession.Current.ObfuscateType(oField.DeclaringType); + + var heapTypes = ClrMDSession.Current.Heap.EnumerateTypes(); + + var target = heapTypes.First(type => type.Name == declaringType); This will iterate all types to find the declaringType everytime ClrDynamic.FindField is called. This probably introduces a performance bottleneck. If there isn't a way to lookup by type name, a cached map should be preprocessed to do this only once ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -274,8 +280,8 @@ private ClrDynamic GetInnerObject(ulong pointer, ClrType type) if (type.IsObjectReference) { - Type.Heap.ReadPointer(pointer, out fieldAddress); - + Type.Heap.Runtime.DataTarget.DataReader.ReadPointer(pointer, out fieldAddress); + //Console.WriteLine(fieldAddress); Remove commented line ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > @@ -284,18 +290,19 @@ private ClrDynamic GetInnerObject(ulong pointer, ClrType type) // Unfortunately, ClrType.GetValue for primitives assumes that the value is boxed, // we decrement PointerSize because it will be added when calling ClrType.GetValue. // ClrMD should be updated in a future version to include ClrType.GetValue(int interior). Is this still true in ClrMD v2? ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > } - return new ClrDynamic(fieldAddress, actualType, !type.IsObjectReference); This space shouldn't have been removed ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > + } + + case ClrElementType.UInt32: + { + return heap.Runtime.DataTarget.DataReader.Read<uint>(unboxedAddress); + } + + case ClrElementType.UInt64: + { + return heap.Runtime.DataTarget.DataReader.Read<ulong>(unboxedAddress); + } + + case ClrElementType.NativeInt: + { + heap.Runtime.DataTarget.DataReader.ReadPointer(unboxedAddress, out var value); + return (long)value; Not sure why the (long) cast is needed here, it looks like this case should be included with the others just below. ------------------------------ In ClrMD.Extensions/ClrDynamic.cs <#26 (comment)> : > if (byteRead != length) throw new InvalidOperationException(string.Format("Expected to read {0} bytes and actually read {1}", length, byteRead)); - return buffer; + return buffer.ToArray(); I think this is creating a new array, it would be better to declare buffer as byte[], there will be an implicit conversion to Span when calling DataReader.Read ------------------------------ In ClrMD.Extensions/ClrMDExtensions.cs <#26 (comment)> : > let type = heap.GetSafeObjectType(address) where set.Contains(type) select new ClrDynamic(address, type); } public static IEnumerable<ClrDynamic> EnumerateDynamicObjects(this ClrHeap heap, string typeName) { + var heapTypes = heap.EnumerateTypes(); + //(from objects in heap.EnumerateObjects() + //group objects by objects.Type into tn + // select (tn.Key) + //); Cleanup commented code ------------------------------ In ClrMD.Extensions/ClrMDExtensions.cs <#26 (comment)> : > if (!typeName.Contains("*")) { var type = - (from t in heap.EnumerateTypes() + (from t in heapTypes It's bad practice to iterate multiple times over an IEnumerable var, it's better to leave it like it was and call heap.EnumerateTypes() multiple times ------------------------------ In ClrMD.Extensions/ClrMDExtensions.cs <#26 (comment)> : > - foreach (ClrStackFrame frame in thread.StackTrace) - builder.AppendLine(frame.DisplayString); + if (frame != null) + builder.AppendLine($"{frame}"); Can frame really be null here? That would be unexpected. This should not use a string interpolation, a single builder.AppendLine(frame.ToString() will be more efficient. Is frame.ToString() similar to the previous frame.DisplayString? ------------------------------ In ClrMD.Extensions/ClrMDExtensions.cs <#26 (comment)> : > + count++; + if (count == 100) break; Why limit this to 100? ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > private ClrMDSession(DataTarget target, string dacFile) { ClrMDSession.Detach(); - if (target.ClrVersions.Count == 0) + if (target.ClrVersions.Count() == 0) Depending of the underlying type, Count() could unnecessary iterate all elements. Should use .Any() instead of Count() == 0. ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > throw new ArgumentException("DataTarget has no clr loaded.", nameof(target)); Target = target; Runtime = dacFile == null ? target.ClrVersions[0].CreateRuntime() : target.ClrVersions[0].CreateRuntime(dacFile); Heap = Runtime.Heap; - //Temp hack until ErrorType is made public - var property = Heap.GetType().GetProperty("ErrorType", BindingFlags.Instance | BindingFlags.NonPublic); - - if (property == null) - throw new InvalidOperationException("Unable to find 'ErrorType' property on ClrHeap."); - - ErrorType = (ClrType)property.GetValue(Heap); + ErrorType = null; //I'm not sure if this is the right thing, but ErrorType doesn't exist in ClrMD 2.0 This will probably break something, I think this hack was needed to match the Type of invalid objects. ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > } - public static ClrMDSession AttachToProcess(int pid, uint millisecondsTimeout = 5000, AttachFlag attachFlag = AttachFlag.Invasive) + public static ClrMDSession AttachToProcess(int pid) AttachFlag no longer exists? Is there an alternative to attach without freezing the process? ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > @@ -123,7 +117,7 @@ public static ClrMDSession AttachToProcess(Process p, uint millisecondsTimeout = Detach(); - DataTarget target = DataTarget.AttachToProcess(p.Id, millisecondsTimeout, attachFlag); + DataTarget target = DataTarget.AttachToProcess(p.Id, true); What is this boolean param here? Is that a replacement for AttachFlag that would be useful to expose? ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > @@ -176,8 +170,16 @@ public IEnumerable<ClrDynamic> EnumerateDynamicObjects(string typeName) var regex = new Regex($"^{Regex.Escape(typeName).Replace("\\*", ".*")}$", RegexOptions.Compiled | RegexOptions.IgnoreCase); + + + var heapTypes = Heap.EnumerateTypes(); + //(from objects in ClrMDSession.Current.Heap.EnumerateObjects() + //group objects by objects.Type into tn + //select (tn.Key) + //); Cleanup commented code ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > var types = - from type in Heap.EnumerateTypes() + from type in heapTypes Looks like heap.EnumerateTypes() can be used directly ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > @@ -322,9 +324,10 @@ private static void TestInvalidComObjectException() { try { - byte[] dummy = new byte[8]; - int bytesRead; - s_currentSession.Runtime.ReadMemory(0, dummy, 8, out bytesRead); + Span<byte> dummy = new byte[8]; + //int bytesRead; + s_currentSession.Target.DataReader.Read(0,dummy); Missing space after comma ------------------------------ In ClrMD.Extensions/ClrMDSession.cs <#26 (comment)> : > @@ -322,9 +324,10 @@ private static void TestInvalidComObjectException() { try { - byte[] dummy = new byte[8]; - int bytesRead; - s_currentSession.Runtime.ReadMemory(0, dummy, 8, out bytesRead); + Span<byte> dummy = new byte[8]; + //int bytesRead; + s_currentSession.Target.DataReader.Read(0,dummy); + //s_currentSession.Runtime.ReadMemory(0, dummy, 8, out bytesRead); Cleanup commented code — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARO76L7DGFNLX73PP6HEE5DTIED2XANCNFSM4S6M74RQ> .