-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding variable tracking throughout assembly printout #2067
Conversation
@@ -37,6 +37,9 @@ | |||
<ProjectReference Include="..\SharpTreeView\ICSharpCode.TreeView.csproj"> | |||
<Private>False</Private> | |||
</ProjectReference> | |||
<Reference Include="ILCompiler.Reflection.ReadyToRun"> | |||
<HintPath>..\..\..\Desktop\r2r\ILCompiler.Reflection.ReadyToRun\bin\Debug\netstandard2.0\ILCompiler.Reflection.ReadyToRun.dll</HintPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. We need to make a PR to dotnet/runtime, get it merged first.
Once we get that, I can work on getting a new nuget package version 1.0.9-alpha
|
||
{ | ||
Dictionary<ulong, UnwindCode> unwindCodes = new Dictionary<ulong, UnwindCode>(); | ||
Dictionary<ulong, HashSet<UnwindCode>> unwindCodes = new Dictionary<ulong, HashSet<UnwindCode>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you finding examples that we have more than one unwind opcode associated with a single code offset? Please let us know. This looks like something is wrong with the compiler or my understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for System.io.FileLoadException.GetMessageForHR in System.private.corelib I am finding 2 opcodes with the same offset of 19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is understood, the root cause is a bug in ILCompiler.Reflection.ReadyToRun
. It would be great that we fix the root cause instead of having this hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should fix it.
foreach (Tuple<DebugInfo, NativeVarInfo> tuple in regSet) { | ||
var debugInfo = tuple.Item1; | ||
var varInfo = tuple.Item2; | ||
if (varInfo.StartOffset < instr.IP - baseInstrIP && varInfo.EndOffset > instr.IP - baseInstrIP && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to double-check with this, I think the StartOffset
should be inclusive, the EndOffset
I am not sure.
DebugInfo.GetPlatformSpecificRegister(debugInfo.Machine, varInfo.VariableLocation.Data1) == usedMemInfo.Base.ToString() && | ||
adjOffset == usedMemInfo.Displacement) { | ||
|
||
output.Write($"; [{usedMemInfo.Base.ToString()}{(negativeOffset ? '-' : '+')}{Math.Abs(stackOffset)}] = {varInfo.Variable.Type} {varInfo.Variable.Index}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to show hex for stackOffset
to be consistent with the disassembly.
ILSpy.Tests/ILSpy.Tests.csproj
Outdated
@@ -49,7 +49,7 @@ | |||
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.7.0-2.final" /> | |||
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic" Version="3.7.0-2.final" /> | |||
<PackageReference Include="NUnit3TestAdapter" Version="3.13.0" /> | |||
<PackageReference Include="System.Collections.Immutable" Version="1.5.0" /> | |||
<PackageReference Include="System.Collections.Immutable" Version="1.7.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be changing these as part of this change? I think they should already be in master.
The commits are getting convoluted, can we squash them?
Some changes I wanted
@christophwille |
output.WriteLine($" Offset: {DebugInfo.GetPlatformSpecificRegister(debugInfo.Machine, varLoc.VariableLocation.Data1)}"); | ||
break; | ||
default: | ||
throw new BadImageFormatException("Unexpected var loc type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we print a user-friendly message instead?
Change how it handle's unexpected var loc types. Co-authored-by: Siegfried Pammer <siegfriedpammer@gmail.com>
This branch allows for variables to be tracked throughout the assembly, if they are of type VLT_STK, VLT_STK_BYREF, VLT_REG, VLT_REG_BYREF, or VLT_REG_FP.
Before it would look like:
With this branches changes it would look like:
This branch uses changes in ILCompiler.Reflection.ReadyToRun, and I'm not entirely sure if those changes are included in this branch completely or if it only added a reference. Also does not track some of the less used variable types such as VLT_REG_REG, as I do not have any examples to test with.