JIT: Throw statement triggers: "has ldstr VM restriction"

[redknightlois]

Description

The JIT is not able to inline this method because of the throw new FormatException("Bad variable size int"); statement with the following error:

generic !!0  (value class System.ReadOnlySpan`1<unsigned int8>,int32,int32&)
Fail Reason: has ldstr VM restriction

A cleanup version of the offending code:

        [MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)]
        public static T ReadVariableSizeInt<T>(ReadOnlySpan<byte> buffer, int pos, out int offset) where T : unmanaged
        {
            if (typeof(T) == typeof(int) || typeof(T) == typeof(uint))
            {
                int ui = 0;

                offset = 0;

                byte b = buffer[pos + 0];
                ui |= (b & 0x7F);
                offset++;
                if ((b & 0x80) == 0)
                    goto End;

                b = buffer[pos + 1];
                ui |= (b & 0x7F) << 7;
                offset++;
                if ((b & 0x80) == 0)
                    goto End;

                b = buffer[pos + 2];
                ui |= (b & 0x7F) << 14;
                offset++;
                if ((b & 0x80) == 0)
                    goto End;

                b = buffer[pos + 3];
                ui |= (b & 0x7F) << 21;
                offset++;
                if ((b & 0x80) == 0)
                    goto End;

                b = buffer[pos + 4];
                ui |= (b & 0x7F) << 28;
                offset++;
                if ((b & 0x80) != 0)
                    throw new FormatException("Bad variable size int");

                End: return (T)(object)ui;
            }

            throw new NotSupportedException($"The type {nameof(T)} is not supported to be written.");
        }

Configuration

.Net 5.0

Analysis

It sounds to me that because the offending line is a throw statement and is to be marked as cold, that restriction should not apply.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[EgorBo]

From my understanding that is intentional: we don't inline methods with string literals inside from other modules, e.g.:

ClassLibrary1:

public class Class1
{
    public string GetString() => "Hello";
}

ConsoleApp1 (references ClassLibrary1 lib):

string Test() => Class1.GetString(); // GetString won't be inlined -- has ldstr VM restriction

because the way interning works, we only can inline it if ClassLibrary doesn't use that literal anywhere. However, I am not sure I fully understand this restriction (perhaps, AOT only?)

[redknightlois]

Yes, I dont know really why it cannot be inlined. But in this case where there is no such construct and also it is essentially a throw it doesn't make much sense that it behaves like that. Or, no method that throws should be able to be inlined.

[JulieLeeMSFT]

CC @AndyAyersMS.

Yes, I dont know really why it cannot be inlined.

[AndyAyersMS]

As @EgorBo says, this is a precaution migrating strings cross-module, in case string interning is active in the caller or callee's module. But seemingly we could relax this, when jitting, most of the time:

runtime/src/coreclr/vm/jitinterface.cpp

Lines 8082 to 8090 in 0177446

	// TODO: We can probably be smarter here if the caller is jitted, as we will
	// know for sure if the inlinee has really no string interning active (currently
	// it's only on in the ngen case (besides requiring the attribute)), but this is getting
	// too subtle. Will only do if somebody screams about it, as bugs here are going to
	// be tough to find
	if ((pOrigCallerModule != pCalleeModule) && pCalleeModule->IsNoStringInterning())
	{
	dwRestrictions |= INLINE_NO_CALLEE_LDSTR;
	}

[redknightlois]

@AndyAyersMS ROFL, you got the scream.

[JulieLeeMSFT]

Pushing out to .NET 7 since it is not likely to be handled by Preview 7.