Skip to content
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

ILSpy incorrectly disassembles System.Buffers.Binary.BinaryPrimitives to empty branches #3166

Open
daiplusplus opened this issue Feb 17, 2024 · 2 comments
Labels
Bug Decompiler The decompiler engine itself

Comments

@daiplusplus
Copy link

daiplusplus commented Feb 17, 2024

Input code

Please provide the input that failed to decompile.

  • Assembly: System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e (.NET 7 SDK 7.0.13)
    • Filesystem location: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.13\System.Private.CoreLib.dll
  • Type: System.Buffers.Binary.BinaryPrimitives
  • Method: ReadInt32BigEndian
  • IL below
  • Raw C# source on GitHub (link points to the 7.0.13 snapshot tag)
.method public hidebysig static 
	int32 ReadInt32BigEndian (
		valuetype System.ReadOnlySpan`1<uint8> source
	) cil managed aggressiveinlining 
{
	// Method begins at RVA 0x64c4fc
	// Header size: 1
	// Code size: 19 (0x13)
	.maxstack 8

	IL_0000: ldsfld bool System.BitConverter::IsLittleEndian
	IL_0005: brtrue.s IL_0007

	IL_0007: ldarg.0
	IL_0008: call !!0 System.Runtime.InteropServices.MemoryMarshal::Read<int32>(valuetype System.ReadOnlySpan`1<uint8>)
	IL_000d: call int32 System.Buffers.Binary.BinaryPrimitives::ReverseEndianness(int32)
	IL_0012: ret
} // end of method BinaryPrimitives::ReadInt32BigEndian

The instruction at IL_0005: brtrue.s IL_0007 doesn't seem right to me... but I'm a bit rusty on my IL right now.

ILSpy disassembles this to:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int ReadInt32BigEndian(ReadOnlySpan<byte> source)
{
	if (!BitConverter.IsLittleEndian)
	{
	}
	return BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(source));
}

The actual C# source is:

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static int ReadInt32BigEndian(ReadOnlySpan<byte> source)
        {
            return BitConverter.IsLittleEndian ?
                ReverseEndianness(MemoryMarshal.Read<int>(source)) :
                MemoryMarshal.Read<int>(source);
        }

Details

Product in use: e.g. ILSpy / ICSharpCode.Decompiler nuget package / VS extension

  • ILSpy version 8.2.0.7535
  • .NET version 6.0.27-servicing.24069.12+80de56dadb3864aec7e8edd3ae32a23aeda08285
@daiplusplus daiplusplus added Bug Decompiler The decompiler engine itself labels Feb 17, 2024
@siegfriedpammer
Copy link
Member

siegfriedpammer commented Feb 17, 2024

If you add the instruction bytes to the IL output, (The setting can be found in Settings > Display > Decompilation view options > last checkbox).

.method /* 06003173 */ public hidebysig static 
	int32 ReadInt32BigEndian (
		valuetype System.ReadOnlySpan`1<uint8> source
	) cil managed aggressiveinlining 
{
	// Method begins at RVA 0x64c5d4
	// Header size: 1
	// Code size: 19 (0x13)
	.maxstack 8

	/* 0x0064C5D5 7E78030004         */ IL_0000: ldsfld bool System.BitConverter::IsLittleEndian /* 04000378 */
	/* 0x0064C5DA 2D00               */ IL_0005: brtrue.s IL_0007

	/* 0x0064C5DC 02                 */ IL_0007: ldarg.0
	/* 0x0064C5DD 284704002B         */ IL_0008: call !!0 System.Runtime.InteropServices.MemoryMarshal::Read<int32>(valuetype System.ReadOnlySpan`1<uint8>) /* 2B000447 */
	/* 0x0064C5E2 2865310006         */ IL_000d: call int32 System.Buffers.Binary.BinaryPrimitives::ReverseEndianness(int32) /* 06003165 */
	/* 0x0064C5E7 2A                 */ IL_0012: ret
} // end of method BinaryPrimitives::ReadInt32BigEndian

You will see that the bytes of the instruction are 0x2D00, where 0x2D is the opcode for brtrue.s:

image
Taken from ECMA-335, 6th edition, June 2012, page 297

and 0x00 is the relative offset to jump to, which basically is "don't jump at all".

Not sure what the C# compiler is trying to do, but it seems pointless. I don't think ILSpy is at fault here. Probably some branch optimization gone wrong in the compiler.

@siegfriedpammer
Copy link
Member

siegfriedpammer commented Feb 17, 2024

Converter.IsLittleEndian is marked with an IntrinsicAttribute, so it seems there is something special going on. May I ask you to open a discussion on the dotnet/runtime repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Decompiler The decompiler engine itself
Projects
None yet
Development

No branches or pull requests

2 participants