Using IInstructionGraph::InsertBefore after applying SingleRetDefaultInstrumentation can fail to update target instructions

[csdahlberg]

Consider an IInstrumentationMethod implementation where ShouldInstrumentMethod indicates that only System::String System::String::Concat(System::String[]) should be instrumented, and where InstrumentMethod includes the following:

    CComPtr<IInstructionFactory> pInstructionFactory;
    pMethodInfo->GetInstructionFactory(&pInstructionFactory);

    CComPtr<IInstructionGraph> pInstructionGraph;
    pMethodInfo->GetInstructions(&pInstructionGraph);

    CComPtr<ISingleRetDefaultInstrumentation> pSingleRet;
    pMethodInfo->GetSingleRetDefaultInstrumentation(&pSingleRet);
    pSingleRet->Initialize(pInstructionGraph);
    pSingleRet->ApplySingleRetDefaultInstrumentation();

    CComPtr<IInstruction> pFirstInstruction;
    pInstructionGraph->GetFirstInstruction(&pFirstInstruction);

    CComPtr<IInstruction> pNopInstr;
    pInstructionFactory->CreateInstruction(Cee_Nop, &pNopInstr);
    pInstructionGraph->InsertBefore(pFirstInstruction, pNopInstr);

In such a case, we are seeing that one of the brtrue instructions has an invalid target instruction. If pNopInstr is not added at the beginning of the method, then there are no invalid instructions.

Without inserting a nop at the beginning of the method:

    /* 0x002             */    IL_0000: ldarg.0 
    /* 0x03A      0x0011 */    IL_0001: brtrue IL_0011
    /* 0x072  0x70000662 */    IL_0006: ldstr "values"
    /* 0x073  0x06001037 */    IL_000B: newobj System.ArgumentNullException::.ctor
    /* 0x07A             */    IL_0010: throw 
    /* 0x002             */    IL_0011: ldarg.0 
    /* 0x08E             */    IL_0012: ldlen 
    /* 0x069             */    IL_0013: conv.i4 
    /* 0x017             */    IL_0014: ldc.i4.1 
    /* 0x03D      0x0047 */    IL_0015: bgt IL_0047
    /* 0x002             */    IL_001A: ldarg.0 
    /* 0x08E             */    IL_001B: ldlen 
    /* 0x039      0x0039 */    IL_001C: brfalse IL_0039
    /* 0x002             */    IL_0021: ldarg.0 
    /* 0x016             */    IL_0022: ldc.i4.0 
    /* 0x09A             */    IL_0023: ldelem.ref 
    /* 0x025             */    IL_0024: dup 
    /* 0x03A      0x003E */    IL_0025: brtrue IL_003E
    /* 0x026             */    IL_002A: pop 
    /* 0x07E  0x04000274 */    IL_002B: ldsfld System.String::Empty
    /* 0x10E       0x009 */    IL_0030: stloc 0x009
    /* 0x038      0x0128 */    IL_0034: br IL_0128
    /* 0x07E  0x04000274 */    IL_0039: ldsfld System.String::Empty
    /* 0x10E       0x009 */    IL_003E: stloc 0x009
    ...

With inserting a nop at the beginning of the method:

    /* 0x000             */    IL_0000: nop 
    /* 0x002             */    IL_0001: ldarg.0 
    /* 0x03A      0x0012 */    IL_0002: brtrue IL_0012
    /* 0x072  0x70000662 */    IL_0007: ldstr "values"
    /* 0x073  0x06001037 */    IL_000C: newobj System.ArgumentNullException::.ctor
    /* 0x07A             */    IL_0011: throw 
    /* 0x002             */    IL_0012: ldarg.0 
    /* 0x08E             */    IL_0013: ldlen 
    /* 0x069             */    IL_0014: conv.i4 
    /* 0x017             */    IL_0015: ldc.i4.1 
    /* 0x03D      0x0048 */    IL_0016: bgt IL_0048
    /* 0x002             */    IL_001B: ldarg.0 
    /* 0x08E             */    IL_001C: ldlen 
    /* 0x039      0x003A */    IL_001D: brfalse IL_003A
    /* 0x002             */    IL_0022: ldarg.0 
    /* 0x016             */    IL_0023: ldc.i4.0 
    /* 0x09A             */    IL_0024: ldelem.ref 
    /* 0x025             */    IL_0025: dup 
    /* 0x03A      0x003E */    IL_0026: brtrue IL_003E
    /* 0x026             */    IL_002B: pop 
    /* 0x07E  0x04000274 */    IL_002C: ldsfld System.String::Empty
    /* 0x10E       0x009 */    IL_0031: stloc 0x009
    /* 0x038      0x0129 */    IL_0035: br IL_0129
    /* 0x07E  0x04000274 */    IL_003A: ldsfld System.String::Empty
    /* 0x10E       0x009 */    IL_003F: stloc 0x009
   ...

Note the IL_0025: brtrue IL_003E instruction that exists without the nop. After inserting the nop, it is changed to IL_0026: brtrue IL_003E, which still targets the now-nonexistent IL_003E instruction (it should have been changed to IL_003F).

string_concat_1_without_nop.log

string_concat_2_with_nop.log

[delmyers]

Thanks, @csdahlberg , for the bug.

Would you mind letting us know what version of the CLR/CoreCLR you are encountering this in? If we are going to reproduce it, we would like to make sure that we are working against the same version of the assembly which you are seeing the problem in.

[csdahlberg]

The example IL was from a .NET Core 2.1 app running on the Microsoft.AspNetCore.App 2.1.22 runtime, but the same behavior has also been experienced with a .NET Core 3.1 app running on Microsoft.AspNetCore.App 3.1.8 and a .NET Framework 4.8 app.