From 40304117b7c15d01f868f95f52e9fe5ac9fb08a9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Apr 2026 05:22:05 +0000 Subject: [PATCH 1/5] Initial plan From 8bfd414c3ecbaa22cc8e428ca3d62b1b33686534 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Apr 2026 05:46:18 +0000 Subject: [PATCH 2/5] Fix SRM branch fixup across BlobBuilder chunk boundaries Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/8495e38c-6615-427e-b675-e825723e8e03 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> --- .../Ecma335/Encoding/ControlFlowBuilder.cs | 7 -- .../Encoding/MethodBodyStreamEncoderTests.cs | 75 +++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs index c161eee8beb05c..13832d4acf79b9 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs @@ -350,13 +350,6 @@ internal void CopyCodeAndFixupBranches(BlobBuilder srcBuilder, BlobBuilder dstBu branch = _branches[branchIndex]; } - // the branch starts at the very end and its operand is in the next blob: - if (srcBlobOffset == srcBlob.Length - 1) - { - srcBlobOffset = operandSize; - break; - } - // skip fake branch operand: srcBlobOffset += operandSize; } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs index 5a60d1210af0cb..857abaf61868bb 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs @@ -533,6 +533,81 @@ public void Branches1() }, dstBuilder.ToArray()); } + [Fact] + public void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBoundaries() + { + var codeBuilder = new BlobBuilder(); + var flowBuilder = new ControlFlowBuilder(); + var il = new InstructionEncoder(codeBuilder, flowBuilder); + var doneLabel = il.DefineLabel(); + + for (int i = 0; i < 30; i++) + { + var skipLabel = il.DefineLabel(); + + il.OpCode(ILOpCode.Ldarg_s); + codeBuilder.WriteByte(3); + + il.OpCode(ILOpCode.Ldarg_s); + codeBuilder.WriteByte(0); + + il.OpCode(ILOpCode.Ceq); + il.Branch(ILOpCode.Brfalse_s, skipLabel); + il.LoadString(MetadataTokens.UserStringHandle(i + 1)); + il.Branch(ILOpCode.Br, doneLabel); + il.MarkLabel(skipLabel); + } + + il.MarkLabel(doneLabel); + il.OpCode(ILOpCode.Ret); + + var ilStream = new BlobBuilder(); + var bodyEncoder = new MethodBodyStreamEncoder(ilStream); + bodyEncoder.AddMethodBody(il, maxStack: 8); + + var streamBytes = ilStream.ToArray(); + Assert.True(streamBytes.Length > 12); + + int pos = 12; + int ldargCount = 0; + while (pos < streamBytes.Length - 1) + { + switch (streamBytes[pos]) + { + case 0x0E: // ldarg.s + ldargCount++; + byte argIndex = streamBytes[pos + 1]; + int expected = (ldargCount % 2 == 1) ? 3 : 0; + Assert.Equal(expected, argIndex); + pos += 2; + break; + + case 0xFE: // 2-byte opcode (ceq) + pos += 2; + break; + + case 0x72: // ldstr + case 0x38: // br + pos += 5; + break; + + case 0x2C: // brfalse.s + pos += 2; + break; + + case 0x2A: // ret + pos += 1; + break; + + default: + Assert.Fail($"Unexpected opcode 0x{streamBytes[pos]:X2} at offset {pos - 12}."); + break; + } + } + + Assert.Equal(60, ldargCount); + } + [Fact] public void BranchErrors() { From 50bfdafaca44eb7c7faf368a61609277ccdd9ad0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Apr 2026 07:32:14 +0000 Subject: [PATCH 3/5] Address code review feedback: robust test parsing and explicit chunk capacity Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f7fd956d-2e3e-46c3-b8fd-4375d3b422f2 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> --- .../Encoding/MethodBodyStreamEncoderTests.cs | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs index 857abaf61868bb..ceea7362d95ccc 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs @@ -534,9 +534,12 @@ public void Branches1() } [Fact] - public void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBoundaries() + public unsafe void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBoundaries() { - var codeBuilder = new BlobBuilder(); + // Explicitly use a 256-byte chunk capacity (the BlobBuilder default) so the test + // remains deterministic even if BlobBuilder's default capacity ever changes. + const int chunkSize = 256; + var codeBuilder = new BlobBuilder(chunkSize); var flowBuilder = new ControlFlowBuilder(); var il = new InstructionEncoder(codeBuilder, flowBuilder); var doneLabel = il.DefineLabel(); @@ -566,23 +569,32 @@ public void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBoundaries() bodyEncoder.AddMethodBody(il, maxStack: 8); var streamBytes = ilStream.ToArray(); - Assert.True(streamBytes.Length > 12); - int pos = 12; + // Use MethodBodyBlock to decode the header and extract the IL bytes, + // rather than hard-coding the fat-header size. + byte[] ilBytes; + fixed (byte* streamPtr = &streamBytes[0]) + { + var body = MethodBodyBlock.Create(new BlobReader(streamPtr, streamBytes.Length)); + ilBytes = body.GetILBytes()!; + } + + int pos = 0; int ldargCount = 0; - while (pos < streamBytes.Length - 1) + while (pos < ilBytes.Length) { - switch (streamBytes[pos]) + switch (ilBytes[pos]) { case 0x0E: // ldarg.s ldargCount++; - byte argIndex = streamBytes[pos + 1]; + byte argIndex = ilBytes[pos + 1]; int expected = (ldargCount % 2 == 1) ? 3 : 0; Assert.Equal(expected, argIndex); pos += 2; break; case 0xFE: // 2-byte opcode (ceq) + Assert.Equal(0x01, ilBytes[pos + 1]); pos += 2; break; @@ -600,11 +612,12 @@ public void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBoundaries() break; default: - Assert.Fail($"Unexpected opcode 0x{streamBytes[pos]:X2} at offset {pos - 12}."); + Assert.Fail($"Unexpected opcode 0x{ilBytes[pos]:X2} at IL offset {pos}."); break; } } + Assert.Equal(ilBytes.Length, pos); Assert.Equal(60, ldargCount); } From 530e1e8549870ff3858b54f46773b866a0a8e5fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 20:52:49 +0000 Subject: [PATCH 4/5] Address review feedback: ILOpCode enum values in switch cases, add chunk-boundary branch test, simplify Debug.Assert Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d5bd07cf-7d67-4dc3-bdce-12158f33e458 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> --- .../Ecma335/Encoding/ControlFlowBuilder.cs | 5 +- .../Encoding/MethodBodyStreamEncoderTests.cs | 80 +++++++++++++++++-- 2 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs index 13832d4acf79b9..f7e75fee70a1e9 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs @@ -292,10 +292,7 @@ internal void CopyCodeAndFixupBranches(BlobBuilder srcBuilder, BlobBuilder dstBu foreach (Blob srcBlob in srcBuilder.GetBlobs()) { - Debug.Assert( - srcBlobOffset == 0 || - srcBlobOffset == 1 && srcBlob.Buffer[0] == 0xff || - srcBlobOffset == 4 && srcBlob.Buffer[0] == 0xff && srcBlob.Buffer[1] == 0xff && srcBlob.Buffer[2] == 0xff && srcBlob.Buffer[3] == 0xff); + Debug.Assert(srcBlobOffset == 0); while (true) { diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs index ceea7362d95ccc..b9a113b0564d98 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs @@ -585,7 +585,7 @@ public unsafe void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBound { switch (ilBytes[pos]) { - case 0x0E: // ldarg.s + case (byte)ILOpCode.Ldarg_s: ldargCount++; byte argIndex = ilBytes[pos + 1]; int expected = (ldargCount % 2 == 1) ? 3 : 0; @@ -593,21 +593,21 @@ public unsafe void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBound pos += 2; break; - case 0xFE: // 2-byte opcode (ceq) - Assert.Equal(0x01, ilBytes[pos + 1]); + case 0xFE: // first byte of a 2-byte opcode (ceq = 0xfe01) + Assert.Equal(unchecked((byte)ILOpCode.Ceq), ilBytes[pos + 1]); pos += 2; break; - case 0x72: // ldstr - case 0x38: // br + case (byte)ILOpCode.Ldstr: + case (byte)ILOpCode.Br: pos += 5; break; - case 0x2C: // brfalse.s + case (byte)ILOpCode.Brfalse_s: pos += 2; break; - case 0x2A: // ret + case (byte)ILOpCode.Ret: pos += 1; break; @@ -621,6 +621,72 @@ public unsafe void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBound Assert.Equal(60, ldargCount); } + [Fact] + public unsafe void AddMethodBody_ShortBranchOperandAtChunkBoundary() + { + // Force a short-branch operand to land at the very last byte of the first + // BlobBuilder chunk (offset 255 in a 256-byte chunk). The removed special-case + // in CopyCodeAndFixupBranches would have advanced srcBlobOffset by operandSize + // BEFORE breaking out of the inner loop, causing it to skip the first byte of the + // next chunk (offset 256) after resuming. + + // Use an explicit 256-byte chunk capacity so the boundary is deterministic. + const int chunkCapacity = 256; + var codeBuilder = new BlobBuilder(chunkCapacity); + var flowBuilder = new ControlFlowBuilder(); + var il = new InstructionEncoder(codeBuilder, flowBuilder); + var skipLabel = il.DefineLabel(); + + // 253 nops + ldc.i4.0 = 254 bytes → brfalse.s opcode at offset 254, + // operand placeholder at offset 255 (last byte of the first chunk). + for (int i = 0; i < 253; i++) + { + il.OpCode(ILOpCode.Nop); + } + + il.OpCode(ILOpCode.Ldc_i4_0); // push 0 so brfalse.s is taken + il.Branch(ILOpCode.Brfalse_s, skipLabel); // opcode at 254, operand at 255 + + // This instruction starts at the very beginning of the second chunk (offset 256). + // Before the fix, the first byte of this chunk would be skipped during branch fixup. + il.OpCode(ILOpCode.Ldc_i4_1); // unreachable, but validates byte at 256 + + il.MarkLabel(skipLabel); // skipLabel at offset 257 + il.OpCode(ILOpCode.Ret); + + var ilStream = new BlobBuilder(); + var bodyEncoder = new MethodBodyStreamEncoder(ilStream); + bodyEncoder.AddMethodBody(il, maxStack: 8); + + var streamBytes = ilStream.ToArray(); + byte[] ilBytes; + fixed (byte* streamPtr = &streamBytes[0]) + { + var body = MethodBodyBlock.Create(new BlobReader(streamPtr, streamBytes.Length)); + ilBytes = body.GetILBytes()!; + } + + Assert.Equal(258, ilBytes.Length); + + // Verify the nops + for (int i = 0; i < 253; i++) + { + Assert.Equal((byte)ILOpCode.Nop, ilBytes[i]); + } + + // Verify ldc.i4.0 + brfalse.s opcode + Assert.Equal((byte)ILOpCode.Ldc_i4_0, ilBytes[253]); + Assert.Equal((byte)ILOpCode.Brfalse_s, ilBytes[254]); + + // Verify the branch operand was correctly fixed up: + // distance = skipLabel(257) - end-of-instruction(256) = 1 + Assert.Equal(1, (sbyte)ilBytes[255]); + + // Verify the first byte of the second chunk was NOT skipped. + Assert.Equal((byte)ILOpCode.Ldc_i4_1, ilBytes[256]); + Assert.Equal((byte)ILOpCode.Ret, ilBytes[257]); + } + [Fact] public void BranchErrors() { From 1642befaa8c28c2687e76a1a8c6a90c629829c28 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 24 Apr 2026 14:20:32 -0700 Subject: [PATCH 5/5] Update MethodBodyStreamEncoderTests.cs --- .../Ecma335/Encoding/MethodBodyStreamEncoderTests.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs index b9a113b0564d98..cacb39dc31624e 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/MethodBodyStreamEncoderTests.cs @@ -625,10 +625,7 @@ public unsafe void AddMethodBody_MixedOpCodeAndCodeBuilderWritesAcrossChunkBound public unsafe void AddMethodBody_ShortBranchOperandAtChunkBoundary() { // Force a short-branch operand to land at the very last byte of the first - // BlobBuilder chunk (offset 255 in a 256-byte chunk). The removed special-case - // in CopyCodeAndFixupBranches would have advanced srcBlobOffset by operandSize - // BEFORE breaking out of the inner loop, causing it to skip the first byte of the - // next chunk (offset 256) after resuming. + // BlobBuilder chunk (offset 255 in a 256-byte chunk). // Use an explicit 256-byte chunk capacity so the boundary is deterministic. const int chunkCapacity = 256;