Skip to content

Add support for MCS 2.6.4 pinned region with array variable#3110

Closed
ElektroKill wants to merge 3 commits intoicsharpcode:masterfrom
ElektroKill:mcs264-pinned-region
Closed

Add support for MCS 2.6.4 pinned region with array variable#3110
ElektroKill wants to merge 3 commits intoicsharpcode:masterfrom
ElektroKill:mcs264-pinned-region

Conversation

@ElektroKill
Copy link
Copy Markdown
Contributor

Link to issue(s) this covers:
N/A

Problem

The decompiler did not support decompiling pinned regions with array variables when the compiler used was MCS 2.6.4.

Before:

public unsafe int MultipleExitsOutOfFixedBlock(int[] arr)
{
	fixed (int* ptr = &System.Runtime.CompilerServices.Unsafe.AsRef<int>((int*)System.Runtime.CompilerServices.Unsafe.AsPointer(ref arr[0])))
	{
		if (*ptr < 0)
		{
			return *ptr;
		}
		if (*ptr == 21)
		{
			return 42;
		}
		if (*ptr == 42)
		{
			Console.WriteLine("outside");
			return 2;
		}
	}
	ptr = null;
	return 1;
}

After:

public unsafe int MultipleExitsOutOfFixedBlock(int[] arr)
{
	fixed (int* ptr = &arr[0])
	{
		if (*ptr < 0)
		{
			return *ptr;
		}
		if (*ptr == 21)
		{
			return 42;
		}
		if (*ptr == 42)
		{
			Console.WriteLine("outside");
			return 2;
		}
	}
	return 1;
}

Solution

  • Added additional code to remove the conv instruction present in the initialization part of the pinned region.
  • Extended the code responsible for removing the unpin stloc to correctly match the inverted condition found in MCS 2.6.4 compiled code.
  • Enabled already present correctness test to run for MCS 2.6.4.

trueInstr is Branch trueBr && trueBr.TargetContainer == sourceContainer &&
reachedEdgesPerBlock[trueBr.TargetBlock.ChildIndex] == 0)
{
unpinBlock = trueBr.TargetBlock;
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.

If I understand correctly, the point of this change is to check both outgoing edges for an if block?
Should innerBlock.Instructions.Count == 2 be innerBlock.Instructions.Count >= 2 instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I used == 2 since in my testing there were always two instructions with one being an if and the second being the fallthrough br. If we were to extend the limit to more than 2 instructions I think it would be necessary to check other instructions for potential outgoing edges which would mean additional logic. though in my testing it seems that the current code is enough.

@dgrunwald dgrunwald closed this in 38e7ab4 Apr 1, 2024
mattsh247 pushed a commit to mattsh247/ILSpy that referenced this pull request Jul 30, 2024
…ray variable

* Added additional code to remove the conv instruction present in the initialization part of the pinned region.
* Extended the code responsible for removing the unpin stloc to correctly match the inverted condition found in MCS 2.6.4 compiled code.
* Enabled already present correctness test to run for MCS 2.6.4.

This is a more generalized version of the fix on PR icsharpcode#3110 proposed by @ElektroKill.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants