Block inlines with different exception wrapping behavior #113849

AndyAyersMS · 2025-03-24T18:37:25Z

Fixes #113815

Fixes dotnet#113815

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)

src/coreclr/vm/jitinterface.cpp: Language not supported

AndyAyersMS · 2025-03-24T18:38:10Z

@jkotas PTAL
cc @dotnet/jit-contrib

jkotas · 2025-03-24T18:46:36Z

src/coreclr/vm/jitinterface.cpp

+            Module* pCalleeModule = pCallee->GetModule();
+            Module* pRootModule = pOrigCaller->GetModule();
+
+            if (pRootModule->IsRuntimeWrapExceptions() != pCalleeModule->IsRuntimeWrapExceptions())


These checks are cached cheap bit tests. We should do them first, and run the more expensive IL header decoder only when the modes do not match.

Also looks like there are still issues with (lack of) an IL header.

Likely I need to handle transient methods here too.

jkotas · 2025-03-24T18:53:01Z

I think we need the same check in crossgen2 for R2R

AndyAyersMS · 2025-03-24T20:30:46Z

I think we need the same check in crossgen2 for R2R

Would the check go into CrossModuleInlineableUncached?

src/coreclr/vm/jitinterface.cpp

jkotas · 2025-03-24T23:20:30Z

Would the check go into CrossModuleInlineableUncached?

I do not see caller and callee in CrossModuleInlineableUncached. I think it needs to be earlier.

AndyAyersMS · 2025-03-25T18:08:35Z

Would the check go into CrossModuleInlineableUncached?

I do not see caller and callee in CrossModuleInlineableUncached. I think it needs to be earlier.

@jkotas I started in on crossgen2 support, but I can't see where custom attributes on modules are recorded, so could use some help finishing it off.

Also I think it would be better to either always return INLINE_FAIL from canInline or else distinguish between INLINE_FAIL and INLINE_NEVER and use the latter to short-circuit some work... at least to remember context-free decisions within the current process.

AndyAyersMS · 2025-03-28T15:33:11Z

@davidwrighton maybe you can give me some pointers? How do we find module attributes in crossgen2? I see support for assembly attributes but nothing for modules.

MichalStrehovsky · 2025-03-31T04:26:00Z

@davidwrighton maybe you can give me some pointers? How do we find module attributes in crossgen2? I see support for assembly attributes but nothing for modules.

We don't have higher level facilities for this in crossgen2, this will have to use System.Reflection.Metadata APIs, something along the lines of:

var reader = rootMod.MetadataReader;
var c = reader.StringComparer;
foreach (var attr in reader.GetModuleDefinition().GetCustomAttributes())
{
    if (reader.GetAttributeNamespaceAndName(attr, out var ns, out var n))
    {
        if (c.Equals(ns, "System.Runtime.CompilerServices") && c.Equals(n, "RuntimeCompatibilityAttribute"))
        {
            var dec = reader.GetCustomAttribute(attr).DecodeValue(new CustomAttributeTypeProvider(mod));
            // Go over dec.NamedArguments and find WrapNonExceptionThrows
        }
    }
}

We'll probably want to cache this on EcmaModule.

AndyAyersMS · 2025-03-31T15:13:24Z

@davidwrighton maybe you can give me some pointers? How do we find module attributes in crossgen2? I see support for assembly attributes but nothing for modules.

We don't have higher level facilities for this in crossgen2, this will have to use System.Reflection.Metadata APIs, something along the lines of:
var reader = rootMod.MetadataReader;
var c = reader.StringComparer;
foreach (var attr in reader.GetModuleDefinition().GetCustomAttributes())
{
    if (reader.GetAttributeNamespaceAndName(attr, out var ns, out var n))
    {
        if (c.Equals(ns, "System.Runtime.CompilerServices") && c.Equals(n, "RuntimeCompatibilityAttribute"))
        {
            var dec = reader.GetCustomAttribute(attr).DecodeValue(new CustomAttributeTypeProvider(mod));
            // Go over dec.NamedArguments and find WrapNonExceptionThrows
        }
    }
}
We'll probably want to cache this on EcmaModule.

Thanks. By "this" do you mean "WrapNonExceptionThrows" or something more general?

davidwrighton · 2025-03-31T22:27:38Z

@AndyAyersMS just WrapNonExceptionThrows. We don't need this concept much, so let's not build a generalized mechanism unless we need to.

jkotas · 2025-04-01T19:02:55Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs

+            EcmaModule rootModule = (root.OwningType as MetadataType)?.Module as EcmaModule;
+            EcmaModule calleeModule = (callee.OwningType as MetadataType)?.Module as EcmaModule;
+
+            // If this inline crosses module boundaries, ensure the modules agree on exception wrapping behavior.


I think this should be directly in CorInfoImpl.cs. It is not a policy decision whether the inlining is allowed for this case. It is required for correctness, so it should not be abstracted by the policy.

Ok. I can then undo some of the other changes, since the policy will no longer need to know the root method.

jkotas · 2025-04-01T19:04:48Z

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs

@@ -16,6 +16,8 @@ public partial class EcmaModule : ModuleDesc
    {
        private readonly PEReader _peReader;
        protected readonly MetadataReader _metadataReader;
+        private bool _isWrapNonExceptionThrowsKnown;
+        private bool _isWrapNonExceptionThrows;


These may need to be volatile.

AndyAyersMS · 2025-04-03T14:55:45Z

@jkotas PTAL.

I haven't added a test case and am not sure this gets properly tested anywhere.

AndyAyersMS · 2025-04-08T22:51:54Z

I finally got around to testing this in crossgen, and the inline is still happening.

It looks like the attribute is actually on the assembly, not on the module?

In my C# calling assembly:

.assembly x
{
  .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilationRelaxationsAttribute::.ctor(int32) = ( 01 00 08 00 00 00 00 00 ) 
  .custom instance void [System.Runtime]System.Runtime.CompilerServices.RuntimeCompatibilityAttribute::.ctor() = ( 01 00 01 00 54 02 16 57 72 61 70 4E 6F 6E 45 78   // ....T..WrapNonEx
                                                                                                                   63 65 70 74 69 6F 6E 54 68 72 6F 77 73 01 )       // ceptionThrows.

And the runtime seems to look it up on the assembly, even though it gets associated with the module...?

runtime/src/coreclr/vm/ceeload.cpp

Lines 1071 to 1099 in a27c3cd

    
           BOOL Module::IsRuntimeWrapExceptions() 
        
           { 
        
               CONTRACTL 
        
               { 
        
                   NOTHROW; 
        
                   if (IsRuntimeWrapExceptionsStatusComputed()) GC_NOTRIGGER; else GC_TRIGGERS; 
        
                   MODE_ANY; 
        
               } 
        
               CONTRACTL_END 
        
               if (!(IsRuntimeWrapExceptionsStatusComputed())) 
        
               { 
        
                   HRESULT hr; 
        
                   BOOL fRuntimeWrapExceptions = FALSE; 
        
                   IMDInternalImport *mdImport = GetAssembly()->GetMDImport(); 
        
                   mdToken token; 
        
                   IfFailGo(mdImport->GetAssemblyFromScope(&token)); 
        
                   const BYTE *pVal; 
        
                   ULONG       cbVal; 
        
                   hr = mdImport->GetCustomAttributeByName(token, 
        
                                   RUNTIMECOMPATIBILITY_TYPE, 
        
                                   (const void**)&pVal, &cbVal); 
        
                   // Parse the attribute 
        
                   if (hr == S_OK)

jkotas · 2025-04-09T00:09:24Z

`Yes, it looks like it. The attribute sources say the attribute target is an assembly:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeCompatibilityAttribute.cs

Line 9 in f7bffcb

    
           [AttributeUsage(AttributeTargets.Assembly, Inherited = false, AllowMultiple = false)]

And the runtime seems to look it up on the assembly, even though it gets associated with the module...?

My guess is that it is a left-over from fragile NGen. Fragile NGen allowed precomputing information associated with Module structure. So in order for the attribute value to be computed at NGen time, caches like this were moved from Assembly to Module structure.

AndyAyersMS · 2025-04-09T00:39:44Z

If I modify my EcmaModule code to look at assembly attributes then the inline is blocked as expected. I can push that change, or would you suggest revising the whole concept to be assembly-based throughout?

jkotas · 2025-04-09T00:45:08Z

I think it is fine to keep it on Module. Assembly and Module are 1:1 in .NET Core. The managed type system even has EcmaAssembly inheriting from EcmaModule.

jkotas · 2025-04-09T00:51:49Z

Are you going to add a test as well?

AndyAyersMS · 2025-04-09T01:22:10Z

Are you going to add a test as well?

I think the existing outerloop R2R pri1 composite tests should cover this (if they also compile the wrappers). But we may need to mark the IL method that throws in the normal runtime tests with aggressive inlining.

Let me try that in a separate PR and see if I can get it to fail.

jkotas · 2025-04-09T05:48:49Z

src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs

+                if (!_isWrapNonExceptionThrowsKnown)
+                {
+                    var reader = MetadataReader;
+                    var c = reader.StringComparer;
+                    foreach (var attr in reader.GetAssemblyDefinition().GetCustomAttributes())
+                    {
+                        if (reader.GetAttributeNamespaceAndName(attr, out var ns, out var n))
+                        {
+                            if (c.Equals(ns, "System.Runtime.CompilerServices") && c.Equals(n, "RuntimeCompatibilityAttribute"))
+                            {
+                                var dec = reader.GetCustomAttribute(attr).DecodeValue(new CustomAttributeTypeProvider(this));
+
+                                foreach (var arg in dec.NamedArguments)
+                                {
+                                    if (arg.Name == "WrapNonExceptionThrows")
+                                    {
+                                        if (!(arg.Value is bool))
+                                            ThrowHelper.ThrowBadImageFormatException();
+                                        _isWrapNonExceptionThrows = (bool)arg.Value;
+                                        break;
+                                    }
+                                }
+                            }
+                        }
+                    }
+
+                    _isWrapNonExceptionThrowsKnown = true;
+                }
+                return _isWrapNonExceptionThrows;
+            }
+        }


Suggested change

if (!_isWrapNonExceptionThrowsKnown)

{

var reader = MetadataReader;

var c = reader.StringComparer;

foreach (var attr in reader.GetAssemblyDefinition().GetCustomAttributes())

{

if (reader.GetAttributeNamespaceAndName(attr, out var ns, out var n))

{

if (c.Equals(ns, "System.Runtime.CompilerServices") && c.Equals(n, "RuntimeCompatibilityAttribute"))

{

var dec = reader.GetCustomAttribute(attr).DecodeValue(new CustomAttributeTypeProvider(this));

foreach (var arg in dec.NamedArguments)

{

if (arg.Name == "WrapNonExceptionThrows")

{

if (!(arg.Value is bool))

ThrowHelper.ThrowBadImageFormatException();

_isWrapNonExceptionThrows = (bool)arg.Value;

break;

}

}

}

}

}

_isWrapNonExceptionThrowsKnown = true;

}

return _isWrapNonExceptionThrows;

}

}

if (!_isWrapNonExceptionThrowsComputed)

{

_isWrapNonExceptionThrows = ComputeIsWrapNonExceptionThrows();

_isWrapNonExceptionThrowsComputed = true;

}

return _isWrapNonExceptionThrows;

}

}

private bool ComputeIsWrapNonExceptionThrows()

{

var reader = MetadataReader;

var c = reader.StringComparer;

foreach (var attr in reader.GetAssemblyDefinition().GetCustomAttributes())

{

if (reader.GetAttributeNamespaceAndName(attr, out var ns, out var n))

{

if (c.Equals(ns, "System.Runtime.CompilerServices") && c.Equals(n, "RuntimeCompatibilityAttribute"))

{

var dec = reader.GetCustomAttribute(attr).DecodeValue(new CustomAttributeTypeProvider(this));

foreach (var arg in dec.NamedArguments)

{

if (arg.Name == "WrapNonExceptionThrows")

{

if (!(arg.Value is bool))

ThrowHelper.ThrowBadImageFormatException();

return (bool)arg.Value;

}

}

}

}

}

return false;

}

The AOT compilers use a pattern with separate method that computes the cached value. It makes the getter cheaper and inlining candidate.

jkotas · 2025-04-09T06:13:01Z

src/coreclr/vm/jitinterface.cpp

+                // We will either find or create transient method details.
+                _ASSERTE(!cxt.HasTransientMethodDetails());
+
+                // IL methods with no RVA indicate there is no implementation defined in metadata.


This is copy&paste from CEEInfo::getMethodInfo. Can this be factored into a method to avoid duplicate code?

AndyAyersMS · 2025-04-09T21:00:10Z

Let me try that in a separate PR and see if I can get it to fail.

I haven't been able to get the expected failure yet in CI. Not sure why yet....

AndyAyersMS · 2025-04-10T00:44:59Z

Let me try that in a separate PR and see if I can get it to fail.

I haven't been able to get the expected failure yet in CI. Not sure why yet....

Aha, the composite mode tests in R2R outerloop do not enable jit optimizations. If I enable optimization, then the composite R2R tests for Directed_3 will fail.

Seems like running these tests with optimizations enabled is desirable, since very few people will build unoptimized R2R images.

Also mark `filter` test entry point as `aggressiveinlining` to create a cross-assembly wrapped exception throw test when crossgen2 runs in composite mode. (see dotnet#113815 / dotnet#113849)

See #113815 / #113849 Not enabled yet on linux.

AndyAyersMS · 2025-04-10T22:28:57Z

Now that #114464 is merged, and I've added aggressive inlining to the IL test case, crossgen2 tests should fail on that test, if the fix doesn't work.

AndyAyersMS · 2025-04-10T22:29:12Z

/azp run runtime-coreclr crossgen2 outerloop

azure-pipelines · 2025-04-10T22:29:25Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2025-04-11T00:55:19Z

@jkotas PTAL...

jkotas

Thanks!

Block inlines with different exception wrapping behavior

872a436

Fixes dotnet#113815

Copilot AI review requested due to automatic review settings March 24, 2025 18:37

Copilot AI reviewed Mar 24, 2025

View reviewed changes

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 24, 2025

dotnet-policy-service bot assigned AndyAyersMS Mar 24, 2025

jkotas reviewed Mar 24, 2025

View reviewed changes

build-analysis bot mentioned this pull request Mar 24, 2025

Segmentation fault running System.Security.Cryptography.Tests #113785

Closed

jkotas reviewed Mar 24, 2025

View reviewed changes

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

revise order of checks; handle transient methods

443eda1

build-analysis bot mentioned this pull request Mar 25, 2025

System.Net.Quic tests timeout #107761

Open

AndyAyersMS added 2 commits March 25, 2025 11:03

first part of crossgen2 support

d40c90f

Merge branch 'main' into Fix113815

e7ef01c

AndyAyersMS requested a review from MichalStrehovsky as a code owner March 25, 2025 18:05

This was referenced Mar 25, 2025

System.Net.Quic.Tests.MsQuicTests.WriteTests failed with "System.Net.Quic.QuicException : The connection timed out from inactivity." #105177

Open

System.Net.Requests test timeout #113883

Closed

AndyAyersMS added 2 commits April 1, 2025 11:51

rest of crossgen2 support

f1c72f3

add bypass for same module inlines

ec2b2cd

jkotas reviewed Apr 1, 2025

View reviewed changes

AndyAyersMS added 2 commits April 1, 2025 13:23

move the check to CorInfoImpl, add volatile

49ff8b7

remove root method arg from policy checks

ccd6a8d

look for the attribute on the assembly, not the module

c5b83c6

build-analysis bot mentioned this pull request Apr 9, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

jkotas reviewed Apr 9, 2025

View reviewed changes

AndyAyersMS mentioned this pull request Apr 9, 2025

Encourage inlining of il throw test #114420

Closed

AndyAyersMS mentioned this pull request Apr 10, 2025

Enable jit optimization in crossgen testing #114464

Merged

AndyAyersMS added 2 commits April 10, 2025 13:23

revise crossgen2 check

6ac2da9

revise jit interface code

d0fd40f

AndyAyersMS added a commit that referenced this pull request Apr 10, 2025

Enable jit optimization in crossgen testing (#114464)

05cd31b

See #113815 / #113849 Not enabled yet on linux.

Merge branch 'main' into Fix113815

78e9cd1

build-analysis bot mentioned this pull request Apr 11, 2025

CI flakiness: mono interpreter build getting killed #114123

Open

jkotas approved these changes Apr 11, 2025

View reviewed changes

AndyAyersMS merged commit 982912a into dotnet:main Apr 11, 2025
145 of 149 checks passed

EgorBo mentioned this pull request Apr 24, 2025

[Perf] Windows/arm64: 4 Improvements on 4/11/2025 11:45:09 PM +00:00 dotnet/perf-autofiling-issues#54353

Closed

AndyAyersMS mentioned this pull request Apr 24, 2025

JIT: De-abstraction in .NET 10 #108913

Open

github-actions bot locked and limited conversation to collaborators May 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block inlines with different exception wrapping behavior #113849

Block inlines with different exception wrapping behavior #113849

AndyAyersMS commented Mar 24, 2025

Copilot AI left a comment

AndyAyersMS commented Mar 24, 2025

jkotas Mar 24, 2025 •

edited

Loading

AndyAyersMS Mar 24, 2025

AndyAyersMS Mar 24, 2025

jkotas commented Mar 24, 2025 •

edited

Loading

AndyAyersMS commented Mar 24, 2025

jkotas commented Mar 24, 2025

AndyAyersMS commented Mar 25, 2025

AndyAyersMS commented Mar 28, 2025

MichalStrehovsky commented Mar 31, 2025

AndyAyersMS commented Mar 31, 2025

davidwrighton commented Mar 31, 2025

jkotas Apr 1, 2025

AndyAyersMS Apr 1, 2025

jkotas Apr 1, 2025

AndyAyersMS commented Apr 3, 2025

AndyAyersMS commented Apr 8, 2025

jkotas commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025

jkotas commented Apr 9, 2025 •

edited

Loading

jkotas commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025 •

edited

Loading

jkotas Apr 9, 2025

jkotas Apr 9, 2025

AndyAyersMS commented Apr 9, 2025

AndyAyersMS commented Apr 10, 2025

AndyAyersMS commented Apr 10, 2025

AndyAyersMS commented Apr 10, 2025

azure-pipelines bot commented Apr 10, 2025

AndyAyersMS commented Apr 11, 2025

jkotas left a comment

Block inlines with different exception wrapping behavior #113849

Block inlines with different exception wrapping behavior #113849

Conversation

AndyAyersMS commented Mar 24, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Mar 24, 2025

jkotas Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

AndyAyersMS Mar 24, 2025

Choose a reason for hiding this comment

AndyAyersMS Mar 24, 2025

Choose a reason for hiding this comment

jkotas commented Mar 24, 2025 • edited Loading

AndyAyersMS commented Mar 24, 2025

jkotas commented Mar 24, 2025

AndyAyersMS commented Mar 25, 2025

AndyAyersMS commented Mar 28, 2025

MichalStrehovsky commented Mar 31, 2025

AndyAyersMS commented Mar 31, 2025

davidwrighton commented Mar 31, 2025

jkotas Apr 1, 2025

Choose a reason for hiding this comment

AndyAyersMS Apr 1, 2025

Choose a reason for hiding this comment

jkotas Apr 1, 2025

Choose a reason for hiding this comment

AndyAyersMS commented Apr 3, 2025

AndyAyersMS commented Apr 8, 2025

jkotas commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025

jkotas commented Apr 9, 2025 • edited Loading

jkotas commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025 • edited Loading

jkotas Apr 9, 2025

Choose a reason for hiding this comment

jkotas Apr 9, 2025

Choose a reason for hiding this comment

AndyAyersMS commented Apr 9, 2025

AndyAyersMS commented Apr 10, 2025

AndyAyersMS commented Apr 10, 2025

AndyAyersMS commented Apr 10, 2025

azure-pipelines bot commented Apr 10, 2025

AndyAyersMS commented Apr 11, 2025

jkotas left a comment

Choose a reason for hiding this comment

jkotas Mar 24, 2025 •

edited

Loading

jkotas commented Mar 24, 2025 •

edited

Loading

jkotas commented Apr 9, 2025 •

edited

Loading

AndyAyersMS commented Apr 9, 2025 •

edited

Loading