changes to allow compatibility with ClrMdV2#26
Open
neilmsimmons wants to merge 1 commit intoJeffCyr:masterfrom
Open
changes to allow compatibility with ClrMdV2#26neilmsimmons wants to merge 1 commit intoJeffCyr:masterfrom
neilmsimmons wants to merge 1 commit intoJeffCyr:masterfrom
Conversation
Author
|
I've been using this with linqpad, it seems to work but it might still need some tidying up |
JeffCyr
requested changes
Apr 11, 2021
Owner
There was a problem hiding this comment.
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)
| throw new InvalidOperationException(string.Format("Type '{0}' is not an array", Type.Name)); | ||
|
|
||
| int arrayLength = Type.GetArrayLength(Address); | ||
| //int arrayLength = Type.GetArrayLength(Address); |
Owner
There was a problem hiding this comment.
Commented code should be removed
| 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(); |
Owner
There was a problem hiding this comment.
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.
|
|
||
| var heapTypes = ClrMDSession.Current.Heap.EnumerateTypes(); | ||
|
|
||
| var target = heapTypes.First(type => type.Name == declaringType); |
Owner
There was a problem hiding this comment.
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
| Type.Heap.ReadPointer(pointer, out fieldAddress); | ||
|
|
||
| Type.Heap.Runtime.DataTarget.DataReader.ReadPointer(pointer, out fieldAddress); | ||
| //Console.WriteLine(fieldAddress); |
| @@ -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). | |||
| } | ||
|
|
||
| public static ClrMDSession AttachToProcess(int pid, uint millisecondsTimeout = 5000, AttachFlag attachFlag = AttachFlag.Invasive) | ||
| public static ClrMDSession AttachToProcess(int pid) |
Owner
There was a problem hiding this comment.
AttachFlag no longer exists? Is there an alternative to attach without freezing the process?
| //(from objects in ClrMDSession.Current.Heap.EnumerateObjects() | ||
| //group objects by objects.Type into tn | ||
| //select (tn.Key) | ||
| //); |
|
|
||
| var types = | ||
| from type in Heap.EnumerateTypes() | ||
| from type in heapTypes |
Owner
There was a problem hiding this comment.
Looks like heap.EnumerateTypes() can be used directly
| s_currentSession.Runtime.ReadMemory(0, dummy, 8, out bytesRead); | ||
| Span<byte> dummy = new byte[8]; | ||
| //int bytesRead; | ||
| s_currentSession.Target.DataReader.Read(0,dummy); |
| Span<byte> dummy = new byte[8]; | ||
| //int bytesRead; | ||
| s_currentSession.Target.DataReader.Read(0,dummy); | ||
| //s_currentSession.Runtime.ReadMemory(0, dummy, 8, out bytesRead); |
Author
|
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>
.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.