ILSpy icon indicating copy to clipboard operation
ILSpy copied to clipboard

Published 20 hours ago • verified publisher icon icsharpcode

Pointer increments and compund assignements not always decompiled in a "pretty" way.

Open ElektroKill opened this issue 2 years ago • 5 comments

Input code

unsafe class UIntCases {
	static void Sample1() {
		uint local = 0;
		uint* r = &local;
		uint g = (*r++) * (*r++);
		Console.WriteLine(g);
	}
	static void Sample1(uint param) {
		uint* r = &param;
		uint g = (*r++) * (*r++);
		Console.WriteLine(g);
	}
	static void Sample5() {
		uint local = 0;
		uint* r = &local;
		uint g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}
	static void Sample5(uint param) {
		uint* r = &param;
		uint g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}
	static void Sample52() {
		uint local = 0;
		uint* r = &local;
		uint g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}
	static void Sample52(uint param) {
		uint* r = &param;
		uint g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}
}
unsafe class ByteCases {
	static void Sample1() {
		byte local = 0;
		byte* r = &local;
		int g = (*r++) * (*r++);
		Console.WriteLine(g);
	}
	static void Sample1(byte param) {
		byte* r = &param;
		int g = (*r++) * (*r++);
		Console.WriteLine(g);
	}
	static void Sample5() {
		byte local = 0;
		byte* r = &local;
		int g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}
	static void Sample5(byte param) {
		byte* r = &param;
		int g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}
	static void Sample52() {
		byte local = 0;
		byte* r = &local;
		int g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}
	static void Sample52(byte param) {
		byte* r = &param;
		int g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}
}

Erroneous output

// ILSpyPointerIncrement.UIntCases
using System;

internal class UIntCases
{
	private unsafe static void Sample1()
	{
		uint local = 0u;
		uint* num = &local;
		uint* r = num + 1;
		uint num2 = *num;
		uint* num3 = r;
		r = num3 + 1;
		Console.WriteLine(num2 * *num3);
	}

	private unsafe static void Sample1(uint param)
	{
		uint* num = &param;
		uint* r = num + 1;
		uint num2 = *num;
		uint* num3 = r;
		r = num3 + 1;
		Console.WriteLine(num2 * *num3);
	}

	private unsafe static void Sample5()
	{
		uint local = 0u;
		uint* r = &local;
		Console.WriteLine((*r += 5u) * (*r += 5u));
	}

	private unsafe static void Sample5(uint param)
	{
		uint* r = &param;
		Console.WriteLine((*r += 5u) * (*r += 5u));
	}

	private unsafe static void Sample52()
	{
		uint local = 0u;
		uint* num = &local + 5;
		uint* r;
		Console.WriteLine(*num * *(r = num + 5));
	}

	private unsafe static void Sample52(uint param)
	{
		uint* num = &param + 5;
		uint* r;
		Console.WriteLine(*num * *(r = num + 5));
	}
}

// ILSpyPointerIncrement.ByteCases
using System;

internal class ByteCases
{
	private unsafe static void Sample1()
	{
		byte local = 0;
		byte* num = &local;
		byte* r = num + 1;
		Console.WriteLine(*num * *(r++));
	}

	private unsafe static void Sample1(byte param)
	{
		byte* num = &param;
		byte* r = num + 1;
		Console.WriteLine(*num * *(r++));
	}

	private unsafe static void Sample5()
	{
		byte local = 0;
		byte* r = &local;
		byte b;
		*r = (b = (byte)(*r + 5));
		byte num = b;
		*r = (b = (byte)(*r + 5));
		Console.WriteLine(num * b);
	}

	private unsafe static void Sample5(byte param)
	{
		byte* r = &param;
		byte b;
		*r = (b = (byte)(*r + 5));
		byte num = b;
		*r = (b = (byte)(*r + 5));
		Console.WriteLine(num * b);
	}

	private unsafe static void Sample52()
	{
		byte local = 0;
		byte* num = &local + 5;
		byte* r;
		Console.WriteLine(*num * *(r = num + 5));
	}

	private unsafe static void Sample52(byte param)
	{
		byte* num = &param + 5;
		byte* r;
		Console.WriteLine(*num * *(r = num + 5));
	}
}

Code does compile. Behavior before and after was not tested.

Details

  • Product in use: ILSpy CI build from https://github.com/icsharpcode/ILSpy/commit/db7d50713851119ef7f09b297ee8bf55b4a48ccd

ElektroKill avatar Feb 03 '22 07:02 ElektroKill

A more "complex" test case:

unsafe class MixedTests {
	static void Mixed1(uint[] y, uint l, uint* e) {
		uint h = 0;
		for (uint i = 0; i < l; i++) {
			*e ^= y[h & 0xf];
			y[h & 0xf] = (y[h & 0xf] ^ (*e++)) + 0x3dbb2819;
			h++;
		}
	}
}

Decompiled code:

internal class MixedTests
{
	private unsafe static void Mixed1(uint[] y, uint l, uint* e)
	{
		uint h = 0u;
		for (uint i = 0u; i < l; i++)
		{
			*e ^= y[h & 0xF];
			uint num = h & 0xF;
			uint num2 = y[h & 0xF];
			uint* num3 = e;
			e = num3 + 1;
			y[num] = (num2 ^ *num3) + 1035675673;
			h++;
		}
	}
}

ElektroKill avatar Feb 03 '22 07:02 ElektroKill

See also #947

siegfriedpammer avatar Feb 03 '22 15:02 siegfriedpammer

I went ahead and tested this with ILSpy 2.4 and the result for some of these test cases is slightly better:

namespace ILSpyPointerIncrement
{
	internal class ByteCases
	{
		private unsafe static void Sample1()
		{
			byte b = 0;
			byte* ptr = &b;
			int value = (int)(*(ptr++) * *(ptr++));
			Console.WriteLine(value);
		}

		private unsafe static void Sample1(byte param)
		{
			byte* ptr = &param;
			int value = (int)(*(ptr++) * *(ptr++));
			Console.WriteLine(value);
		}

		private unsafe static void Sample5()
		{
			byte b = 0;
			byte* ptr = &b;
			byte* expr_08 = ptr;
			int arg_1B_0 = (int)(*expr_08 += 5);
			byte* expr_12 = ptr;
			int value = arg_1B_0 * (int)(*expr_12 += 5);
			Console.WriteLine(value);
		}

		private unsafe static void Sample5(byte param)
		{
			byte* ptr = &param;
			byte* expr_06 = ptr;
			int arg_19_0 = (int)(*expr_06 += 5);
			byte* expr_10 = ptr;
			int value = arg_19_0 * (int)(*expr_10 += 5);
			Console.WriteLine(value);
		}

		private unsafe static void Sample52()
		{
			byte b = 0;
			byte* ptr = &b;
			int value = (int)(*(ptr += 5) * ptr[5]);
			Console.WriteLine(value);
		}

		private unsafe static void Sample52(byte param)
		{
			byte* ptr = &param;
			int value = (int)(*(ptr += 5) * ptr[5]);
			Console.WriteLine(value);
		}
	}
}

namespace ILSpyPointerIncrement
{
	internal class MixedTests
	{
		private unsafe static void Mixed1(uint[] y, uint l, uint* e)
		{
			uint num = 0u;
			for (uint num2 = 0u; num2 < l; num2 += 1u)
			{
				*e ^= y[(int)(num & 15u)];
				y[(int)(num & 15u)] = (y[(int)(num & 15u)] ^ *(e++)) + 1035675673u;
				num += 1u;
			}
		}
	}
}

namespace ILSpyPointerIncrement
{
	internal class UIntCases
	{
		private unsafe static void Sample1()
		{
			uint num = 0u;
			uint* ptr = &num;
			uint value = *(ptr++) * *(ptr++);
			Console.WriteLine(value);
		}

		private unsafe static void Sample1(uint param)
		{
			uint* ptr = &param;
			uint value = *(ptr++) * *(ptr++);
			Console.WriteLine(value);
		}

		private unsafe static void Sample5()
		{
			uint num = 0u;
			uint* ptr = &num;
			uint value = (*ptr += 5u) * (*ptr += 5u);
			Console.WriteLine(value);
		}

		private unsafe static void Sample5(uint param)
		{
			uint* ptr = &param;
			uint value = (*ptr += 5u) * (*ptr += 5u);
			Console.WriteLine(value);
		}

		private unsafe static void Sample52()
		{
			uint num = 0u;
			uint* ptr = &num;
			uint value = *(ptr += 5) * ptr[5];
			Console.WriteLine(value);
		}

		private unsafe static void Sample52(uint param)
		{
			uint* ptr = &param;
			uint value = *(ptr += 5) * ptr[5];
			Console.WriteLine(value);
		}
	}
}

The old decompiler does a better job with the Mixed1 method as well as the Sample1 methods.

ElektroKill avatar Feb 04 '22 18:02 ElektroKill

Hello, I've gone ahead and extended the test cases. I've also concluded that there is no difference in decompilation results between pointers to locals and pointers to parameters.

Updated test cases:

unsafe partial class PointerCompoundAssignTests {
	static void ByteParameterReferencePostIncrementBeforeDereference(byte param) {
		byte* r = &param;
		int g = (*r++) * (*r++);
		Console.WriteLine(g);
	}

	static void ByteParameterReferencePostIncrementAfterDereference(byte param) {
		byte* r = &param;
		int g = ((*r)++) * ((*r)++);
		Console.WriteLine(g);
	}

	static void ByteParameterReferencePreIncrementBeforeDereference(byte param) {
		byte* r = &param;
		int g = (*++r) * (*++r);
		Console.WriteLine(g);
	}

	static void ByteParameterReferencePreIncrementAfterDereference(byte param) {
		byte* r = &param;
		int g = (++(*r)) * (++(*r));
		Console.WriteLine(g);
	}

	static void ByteParameterReferenceCompundAssignAfterDereference(byte param) {
		byte* r = &param;
		int g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}

	static void ByteParameterReferenceCompundAssignBeforeDereference(byte param) {
		byte* r = &param;
		int g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}

	static void UIntParameterReferencePostIncrementBeforeDereference(uint param) {
		uint* r = &param;
		uint g = (*r++) * (*r++);
		Console.WriteLine(g);
	}

	static void UIntParameterReferencePostIncrementAfterDereference(uint param) {
		uint* r = &param;
		uint g = ((*r)++) * ((*r)++);
		Console.WriteLine(g);
	}

	static void UIntParameterReferencePreIncrementBeforeDereference(uint param) {
		uint* r = &param;
		uint g = (*++r) * (*++r);
		Console.WriteLine(g);
	}

	static void UIntParameterReferencePreIncrementAfterDereference(uint param) {
		uint* r = &param;
		uint g = (++(*r)) * (++(*r));
		Console.WriteLine(g);
	}

	static void UIntParameterReferenceCompundAssignAfterDereference(uint param) {
		uint* r = &param;
		uint g = (*r += 5) * (*r += 5);
		Console.WriteLine(g);
	}

	static void UIntParameterReferenceCompundAssignBeforeDereference(uint param) {
		uint* r = &param;
		uint g = (*(r += 5)) * (*(r += 5));
		Console.WriteLine(g);
	}
}

unsafe class MixedTests {
	static void Mixed1(uint[] y, uint l, uint* e) {
		uint h = 0;
		for (uint i = 0; i < l; i++) {
			*e ^= y[h & 0xf];
			y[h & 0xf] = (y[h & 0xf] ^ (*e++)) + 0x3dbb2819;
			h++;
		}
	}
}

Decompilation output for the test cases above:

With Always inline local variables if possible option disabled:

internal class PointerCompoundAssignTests
{
	private unsafe static void ByteParameterReferencePostIncrementBeforeDereference(byte param)
	{
		byte* r = &param;
		int g = *(r++) * *(r++);
		Console.WriteLine(g);
	}

	private unsafe static void ByteParameterReferencePostIncrementAfterDereference(byte param)
	{
		byte* r = &param;
		byte b = *r;
		*r = (byte)(b + 1);
		byte num = b;
		b = *r;
		*r = (byte)(b + 1);
		int g = num * b;
		Console.WriteLine(g);
	}

	private unsafe static void ByteParameterReferencePreIncrementBeforeDereference(byte param)
	{
		byte* r = &param;
		int g = *(++r) * *(++r);
		Console.WriteLine(g);
	}

	private unsafe static void ByteParameterReferencePreIncrementAfterDereference(byte param)
	{
		byte* r = &param;
		byte b = (byte)(*r + 1);
		*r = b;
		byte num = b;
		b = (byte)(*r + 1);
		*r = b;
		int g = num * b;
		Console.WriteLine(g);
	}

	private unsafe static void ByteParameterReferenceCompundAssignAfterDereference(byte param)
	{
		byte* r = &param;
		byte b;
		*r = (b = (byte)(*r + 5));
		byte num = b;
		*r = (b = (byte)(*r + 5));
		int g = num * b;
		Console.WriteLine(g);
	}

	private unsafe static void ByteParameterReferenceCompundAssignBeforeDereference(byte param)
	{
		byte* r = &param;
		int g = *(r += 5) * *(r += 5);
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferencePostIncrementBeforeDereference(uint param)
	{
		uint* r = &param;
		uint* intPtr = r;
		r = intPtr + 1;
		uint num = *intPtr;
		uint* intPtr2 = r;
		r = intPtr2 + 1;
		uint g = num * *intPtr2;
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferencePostIncrementAfterDereference(uint param)
	{
		uint* r = &param;
		uint g = (*r)++ * (*r)++;
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferencePreIncrementBeforeDereference(uint param)
	{
		uint* r = &param;
		uint g = *(++r) * *(++r);
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferencePreIncrementAfterDereference(uint param)
	{
		uint* r = &param;
		uint g = ++(*r) * ++(*r);
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferenceCompundAssignAfterDereference(uint param)
	{
		uint* r = &param;
		uint g = (*r += 5u) * (*r += 5u);
		Console.WriteLine(g);
	}

	private unsafe static void UIntParameterReferenceCompundAssignBeforeDereference(uint param)
	{
		uint* r = &param;
		uint g = *(r += 5) * *(r += 5);
		Console.WriteLine(g);
	}
}

internal class MixedTests
{
	private unsafe static void Mixed1(uint[] y, uint l, uint* e)
	{
		uint h = 0u;
		for (uint i = 0u; i < l; i++)
		{
			*e ^= y[h & 0xF];
			uint num = h & 0xF;
			uint num2 = y[h & 0xF];
			uint* intPtr = e;
			e = intPtr + 1;
			y[num] = (num2 ^ *intPtr) + 1035675673;
			h++;
		}
	}
}

With Always inline local variables if possible option enabled:

internal class PointerCompoundAssignTests
{
	private unsafe static void ByteParameterReferencePostIncrementBeforeDereference(byte param)
	{
		byte* intPtr = &param;
		byte* r = intPtr + 1;
		Console.WriteLine(*intPtr * *(r++));
	}

	private unsafe static void ByteParameterReferencePostIncrementAfterDereference(byte param)
	{
		byte* r = &param;
		byte b = *r;
		*r = (byte)(b + 1);
		byte num = b;
		b = *r;
		*r = (byte)(b + 1);
		Console.WriteLine(num * b);
	}

	private unsafe static void ByteParameterReferencePreIncrementBeforeDereference(byte param)
	{
		byte* intPtr = &param + 1;
		byte* r;
		Console.WriteLine(*intPtr * *(r = intPtr + 1));
	}

	private unsafe static void ByteParameterReferencePreIncrementAfterDereference(byte param)
	{
		byte* r = &param;
		byte b = (byte)(*r + 1);
		*r = b;
		byte num = b;
		b = (byte)(*r + 1);
		*r = b;
		Console.WriteLine(num * b);
	}

	private unsafe static void ByteParameterReferenceCompundAssignAfterDereference(byte param)
	{
		byte* r = &param;
		byte b;
		*r = (b = (byte)(*r + 5));
		byte num = b;
		*r = (b = (byte)(*r + 5));
		Console.WriteLine(num * b);
	}

	private unsafe static void ByteParameterReferenceCompundAssignBeforeDereference(byte param)
	{
		byte* intPtr = &param + 5;
		byte* r;
		Console.WriteLine(*intPtr * *(r = intPtr + 5));
	}

	private unsafe static void UIntParameterReferencePostIncrementBeforeDereference(uint param)
	{
		uint* intPtr = &param;
		uint* r = intPtr + 1;
		uint num = *intPtr;
		uint* intPtr2 = r;
		r = intPtr2 + 1;
		Console.WriteLine(num * *intPtr2);
	}

	private unsafe static void UIntParameterReferencePostIncrementAfterDereference(uint param)
	{
		uint* r = &param;
		Console.WriteLine((*r)++ * (*r)++);
	}

	private unsafe static void UIntParameterReferencePreIncrementBeforeDereference(uint param)
	{
		uint* intPtr = &param + 1;
		uint* r;
		Console.WriteLine(*intPtr * *(r = intPtr + 1));
	}

	private unsafe static void UIntParameterReferencePreIncrementAfterDereference(uint param)
	{
		uint* r = &param;
		Console.WriteLine(++(*r) * ++(*r));
	}

	private unsafe static void UIntParameterReferenceCompundAssignAfterDereference(uint param)
	{
		uint* r = &param;
		Console.WriteLine((*r += 5u) * (*r += 5u));
	}

	private unsafe static void UIntParameterReferenceCompundAssignBeforeDereference(uint param)
	{
		uint* intPtr = &param + 5;
		uint* r;
		Console.WriteLine(*intPtr * *(r = intPtr + 5));
	}
}

internal class MixedTests
{
	private unsafe static void Mixed1(uint[] y, uint l, uint* e)
	{
		uint h = 0u;
		for (uint i = 0u; i < l; i++)
		{
			*e ^= y[h & 0xF];
			uint num = h & 0xF;
			uint num2 = y[h & 0xF];
			uint* intPtr = e;
			e = intPtr + 1;
			y[num] = (num2 ^ *intPtr) + 1035675673;
			h++;
		}
	}
}

Summary of testing results:

The decompiler fails to produce a pretty result for MixedTests.Mixed1 no matter the local inlining option. When the local inlining option is disabled, the following test cases yield "not pretty" results:

  • ByteParameterReferencePostIncrementAfterDereference
  • ByteParameterReferencePreIncrementAfterDereference
  • ByteParameterReferenceCompundAssignAfterDereference
  • UIntParameterReferencePostIncrementBeforeDereference

When the local inlining option is enabled, the following test cases yield "not pretty" results:

  • ByteParameterReferencePostIncrementBeforeDereference
  • ByteParameterReferencePostIncrementAfterDereference
  • ByteParameterReferencePreIncrementBeforeDereference
  • ByteParameterReferencePreIncrementAfterDereference
  • ByteParameterReferenceCompundAssignAfterDereference
  • ByteParameterReferenceCompundAssignBeforeDereference
  • UIntParameterReferencePostIncrementBeforeDereference
  • UIntParameterReferencePreIncrementBeforeDereference
  • UIntParameterReferenceCompundAssignBeforeDereference

From these results, it is safe to assume that the Always inline local variables if possible negatively impacts the decompilation results when dealing with post/pre increments and compound assignments with pointers involved. Is this negative impact of Always inline local variables if possible intended behavior or is it a bug in the decompiler causing these inferior decompilation results?

ElektroKill avatar Aug 03 '22 17:08 ElektroKill

Is there a chance for this issue to be fixed in ILSpy 8?

ElektroKill avatar Sep 17 '22 12:09 ElektroKill

I came up with a potential fix for the UIntParameterReferencePostIncrementBeforeDereference test code not decompiling in into a post-increment. The first if condition which contains a check which verifies whether the binary.Right instruction is ldc.i4 1 could be modified to use PointerArithmeticOffset.Detect to detect the proper operation for pointer compound assignments. https://github.com/icsharpcode/ILSpy/blob/3d117a5beac9e959d211d12295dd6cdf8e9cb77b/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L872-L884 If this is applied then the assertions in ExpressionBuilder.HandleCompoundAssignment would have to be modified as well. https://github.com/icsharpcode/ILSpy/blob/3d117a5beac9e959d211d12295dd6cdf8e9cb77b/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs#L1901-L1920 The second assertion would have to be modified to also use PointerArithmeticOffset when we are dealing with pointer post-increments. Another solution would be to set the Value of the NumericCompoundAssign instruction to the output of PointerArithmeticOffset.Detect to avoid the need for modifying the assertions in ExpressionBuilder. Not sure which approach here is the best.

Please let me know if this is an appropriate fix for the issue with UIntParameterReferencePostIncrementBeforeDereference. If it is I can open a PR with these changes.

ElektroKill avatar Dec 03 '22 10:12 ElektroKill

The first approach makes sense.

NumericCompoundAssign is (like most of the ILAst) supposed to have IL semantics, so it doesn't make sense to use the number of elements there.

dgrunwald avatar Dec 03 '22 13:12 dgrunwald

Hi, I have identified the cause for the ByteParameterReferencePostIncrementAfterDereference, ByteParameterReferencePreIncrementAfterDereference, and ByteParameterReferenceCompundAssignAfterDereference test methods not decompiling nicely.

The issue is the checks performed in the IsImplicitTruncation method and ValidateCompoundAssign methods and the code generated by the compiler for these methods. These methods perform a check based on the primitive type of instruction in these places: https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L543-L546 https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L576-L579 https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L235-L236 This direct comparison is problematic if we look at the code generated by the C# compiler for these methods. image In the raw IL it is possible to observe a mismatch between primitive types which causes the check to fail. This mismatch is caused by the fact that IL does not have unsigned variations of stind. This issue with the checks cannot be observed on UInt32 compound assignments as it is not a small integer type and is thus excluded from the IsImplcitTruncation checks. The compound assignments on UInt32 also do not include a conv so the condition in ValidateCompoundAssign is skipped. This means that this problem not only occurs with byte but with any unsigned small integer type and char. https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L516-L522 This problem is the reason for the ugly decompilation on all 3 test methods mentioned at the start of the comment due to the mismatch in primitive types.

ElektroKill avatar Feb 09 '23 11:02 ElektroKill

A potential fix for the problem identified above:

Change conditions to only compare type size and not whether it is signed or unsigned:

  1. https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L543-L546 Changed return statement to: 
    return conv.TargetType.GetSize() != type.ToPrimitiveType().GetSize();
  2. https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L576-L579 Change return statement to: 
    return !(inferredType.GetSize() <= type.GetSize());
  3. https://github.com/icsharpcode/ILSpy/blob/4ebe075e5859939463ae420446f024f10c3bf077/ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs#L235-L236 Change if statement to: 
    if (conv != null && !(conv.TargetType.GetSize() == targetType.ToPrimitiveType().GetSize() && conv.CheckForOverflow == binary.CheckForOverflow))
	return false; // conv does not match binary operation

All tests pass with these changes applied.

Not sure if this is the right way to approach this issue. I'm open to feedback on my suggestion. If this fix is appropriate I can open a pull request with it and some test cases for it.

ElektroKill avatar Feb 11 '23 10:02 ElektroKill

I'm still awaiting feedback on my proposed fix for the issue :p

ElektroKill avatar Apr 15 '23 12:04 ElektroKill

1+2: I don't think those suggestions are correct. IsImplicitTruncation should return true only if stobj type(..., value) evaluates to the same value as value. Note that if type is a small integer, stobj will truncate value, then store it, then widen again back to I4 based on the signedness of type. stobj System.Byte(..., conv i1(ldc.i4 130)) will evaluate to the I4 130, but the input value (the conv) evaluates to the I4 -126 instead. So the function must return false.

dgrunwald avatar Apr 19 '23 16:04 dgrunwald

On the IL level, stobj only has a single form for both signed and unsigned bytes, and that form only truncates and doesn't evaluate to any result at all. But in order to support it as an inline expression, we've extended the semantics, adding the re-widening, which matches how C# handles an assignment expression: call F(stobj sbyte(v, ldc.i4 130)) will turn into F(v = -126), so we can't use that if the original IL called F(130).

Removing the sign comparison would make the transform incorrect. If the transform notices that only a sign mismatch prevents it from working, the transform could change the type of the stobj (changing only the sign has no effect if the result of the stobj isn't used yet), and then successfully apply the transformation. But without changing the type of the stobj, your proposal isn't correct.

dgrunwald avatar Apr 19 '23 17:04 dgrunwald

Would the following approach be valid?

  1. Leave IsImplicitTruncation like it is called now but extend it to return more information about the truncation.
  2. Call it the first time with the same parameters as currently done
  3. If the only reason for the function returning true is a mismatch in the sign of the type, change the stobj type to the opposite sign type and continue as if it was not a truncation.

ElektroKill avatar Apr 19 '23 17:04 ElektroKill

Yes, that approach should work.

dgrunwald avatar Apr 19 '23 18:04 dgrunwald