[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437

lambdageek · 2023-11-06T21:09:46Z

Interfaces can have static generic methods, for example. They don't have a vt_slot.

When building an IMT slot, we need to collect all the interface methods that map to a particular IMT slot along with their
implementing vtable entries. To do that, vt_slot starts at the interface offset of a particular interface and keeps incrementing as we iterate over the methods of the interface. It is crtitical that vt_slot is accurate - otherwise we may dispatch the interface call to the wrong virtual method.

Fixes #93770

Also remove the extra_interfaces argument from build_imt_slots - it was used only for appdomain support (transparent proxy objects implemented extra interfaces).

Also fixup some ifdef'd debugging code.

Interfaces can have static generic methods, for example. They don't have a vt_slot. When building an IMT slot, we need to collect all the interface methods that map to a particular IMT slot along with their implementing vtable entries. To do that, vt_slot starts at the interface offset of a particular interface and keeps incrementing as we iterate over the methods of the interface. It is crtitical that vt_slot is accurate - otherwise we may dispatch the interface call to the wrong virtual method.

the extra_interfaces argument was used to implement additional interfaces on cross-domain transparent proxy objects.

lambdageek · 2023-11-06T21:13:57Z

It's frustrating that we have to do this manual counting of vt_slot. It woudl be great if every method we could just say vt_slot = interface_offset + method->slot. But unfortunately, class-init.c also sets the method->slot for for static virtual methods, while built_imt_slots doesn't look at static virtual methods when assigning slots for interfaces.

runtime/src/mono/mono/metadata/class-init.c

Lines 3608 to 3620 in 5184067

    
           if (MONO_CLASS_IS_INTERFACE_INTERNAL (klass)) { 
        
           	int slot = 0; 
        
           	/*Only assign slots to virtual methods as interfaces are allowed to have static methods.*/ 
        
           	for (i = 0; i < count; ++i) { 
        
           		if (methods [i]->flags & METHOD_ATTRIBUTE_VIRTUAL) 
        
           		{ 
        
           			if (method_is_reabstracted (methods[i]->flags)) { 
        
           				if (!methods [i]->is_inflated) 
        
           					mono_method_set_is_reabstracted (methods [i]); 
        
           				continue; 
        
           			} 
        
           			methods [i]->slot = slot++; 
        
           		}

I'm not actually convinced that build_imt_method is completely right about static virtual functions, although it's hard to actually produce a failing test case for this code. (the original issue had some failure in EFCore and trying to make a smaller testcase from it didn't really work out)

lewing · 2023-11-06T22:25:31Z

two curl failures in the mono lanes

https://dev.azure.com/dnceng-public/public/_build/results?buildId=461322&view=logs&j=7f49df26-8126-5de3-bf2f-6ac6bde01830&t=af2c051c-0c65-59f6-e97b-23ca21856a55&l=15

/bin/bash --noprofile --norc /Users/runner/work/_temp/b2a99c01-a851-429e-974f-76d8d10baf16.sh
Downloading 'https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.sh'
curl: (35) Send failure: Broken pipe
Curl failed; dumping some information about dotnet.microsoft.com for later investigation
write:errno=54
CONNECTED(00000006)
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 0 bytes and written 322 bytes
Verification: OK
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---
##[error]Bash exited with code '1'

lambdageek · 2023-11-07T14:42:19Z

/backport to release/7.0-staging

lambdageek · 2023-11-07T14:42:30Z

/backport to release/6.0-staging

github-actions · 2023-11-07T14:42:31Z

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607

github-actions · 2023-11-07T14:42:54Z

Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881

lambdageek · 2023-11-07T14:43:05Z

/backport to release/8.0-staging

github-actions · 2023-11-07T14:43:17Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723

tmds · 2023-11-07T14:46:10Z

@lambdageek I'm going to apply this change to dotnet/dotnet release/8.0.1xx, run the aspnetcore test suite against it, and share the results. It will take about 4 hours (if all goes well).

lambdageek · 2023-11-07T14:49:17Z

I'm going to try one more idea to get make a test case for this (basically try to force an IMT collision by having 20 interface methods (since there are 19 slots))

lambdageek · 2023-11-07T17:18:07Z

/backport to release/8.0-staging

lambdageek · 2023-11-07T17:18:16Z

/backport to release/7.0-staging

github-actions · 2023-11-07T17:18:19Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658

github-actions · 2023-11-07T17:18:27Z

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251

tmds · 2023-11-07T19:29:23Z

run the aspnetcore test suite against it, and share the results. It will take about 4 hours (if all goes well).

The ef core issue is fixed by this.

There are still about a 100 failures with mono on x64.

This ArgumentNullException happens in several tests:

System.ArgumentNullException: System.ArgumentNullException : Value cannot be null. (Parameter 'attributeType')
        at System.Reflection.CustomAttribute.GetCustomAttributes(ICustomAttributeProvider obj, Type attributeType, Boolean inherit)
at System.Reflection.RuntimeParameterInfo.GetCustomAttributes(Type attributeType, Boolean inherit)
at Microsoft.AspNetCore.Http.PropertyAsParameterInfo.GetCustomAttributes(Type attributeType, Boolean inherit) in /_/src/Shared/PropertyAsParameterInfo.cs:line 142
at System.Reflection.CustomAttribute.GetCustomAttributesBase(ICustomAttributeProvider obj, Type attributeType, Boolean inheritedOnly)
at System.Reflection.CustomAttribute.GetCustomAttributes(ICustomAttributeProvider obj, Type attributeType, Boolean inherit)
at System.Reflection.CustomAttribute.GetCustomAttributes(ICustomAttributeProvider obj, Boolean inherit)
at System.Attribute.GetCustomAttributes(ParameterInfo element)
at System.Reflection.CustomAttributeExtensions.GetCustomAttributes(ParameterInfo element)
at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArgument(ParameterInfo parameter, RequestDelegateFactoryContext factoryContext) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 690
at Microsoft.AspNetCore.Http.RequestDelegateFactory.BindParameterFromProperties(ParameterInfo parameter, RequestDelegateFactoryContext factoryContext) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 1527
at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArgument(ParameterInfo parameter, RequestDelegateFactoryContext factoryContext) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 791
at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArguments(ParameterInfo[] parameters, RequestDelegateFactoryContext factoryContext) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 640
at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArgumentsAndInferMetadata(MethodInfo methodInfo, RequestDelegateFactoryContext factoryContext) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 394
at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression targetExpression, RequestDelegateFactoryContext factoryContext, Expression`1 targetFactory) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 339
at Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(Delegate handler, RequestDelegateFactoryOptions options, RequestDelegateMetadataResult metadataResult) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 190
at Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(Delegate handler, RequestDelegateFactoryOptions options) in /_/src/Http/Http.Extensions/src/RequestDelegateFactory.cs:line 161
at Microsoft.AspNetCore.Routing.Internal.RequestDelegateFactoryTests.RequestDelegatePopulatesParametersFromServiceWithAndWithoutAttribute(Delegate action) in /_/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs:line 1184

The SignalR tests show another issue that is vtable related. It's not a regression caused by this PR (the tests are also failing with the rc2 build).

Xunit.Sdk.FailException: Assert.Fail(): 1 error(s) logged.
Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher - FailedInvokingHubMethod - Failed to invoke hub method 'ClientSendMethod'.
===================
System.TypeLoadException: VTable setup of type Microsoft.AspNetCore.SignalR.TypedClientBuilder.ITestImpl failed
 at System.RuntimeType.GetMethodsByName(String name, BindingFlags bindingAttr, MemberListType listType, RuntimeType reflectedType)
 at System.RuntimeType.GetMethodCandidates(String name, BindingFlags bindingAttr, CallingConventions callConv, Type[] types, Int32 genericParamCount, Boolean allowPrefixLookup)
 at System.RuntimeType.GetMethodImpl(String name, Int32 genericParamCount, BindingFlags bindingAttr, Binder binder, CallingConventions callConv, Type[] types, ParameterModifier[] modifiers)
 at System.RuntimeType.GetMethodImpl(String name, BindingFlags bindingAttr, Binder binder, CallingConventions callConvention, Type[] types, ParameterModifier[] modifiers)
 at System.Type.GetMethod(String name, BindingFlags bindingAttr)
 at Microsoft.AspNetCore.SignalR.Internal.TypedClientBuilder`1[[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]].GenerateClientBuilder() in /_/src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs:line 43
 at System.Lazy`1[[System.Func`2[[Microsoft.AspNetCore.SignalR.IClientProxy, Microsoft.AspNetCore.SignalR.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ViaFactory(LazyThreadSafetyMode mode)
--- End of stack trace from previous location ---
 at System.LazyHelper.ThrowException()
 at System.Lazy`1[[System.Func`2[[Microsoft.AspNetCore.SignalR.IClientProxy, Microsoft.AspNetCore.SignalR.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CreateValue()
 at System.Lazy`1[[System.Func`2[[Microsoft.AspNetCore.SignalR.IClientProxy, Microsoft.AspNetCore.SignalR.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value()
 at Microsoft.AspNetCore.SignalR.Internal.TypedClientBuilder`1[[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]].Build(IClientProxy proxy) in /_/src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs:line 25
 at Microsoft.AspNetCore.SignalR.Internal.TypedHubClients`1[[Microsoft.AspNetCore.SignalR.Tests.ITest, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]].User(String userId) in /_/src/SignalR/server/Core/src/Internal/TypedHubClients.cs:line 52
 at Microsoft.AspNetCore.SignalR.Tests.HubT.ClientSendMethod(String userId, String message) in /_/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs:line 490
 at System.Object.lambda_method29545(Closure , Object , Object[] )
 at Microsoft.Extensions.Internal.ObjectMethodExecutor.Execute(Object target, Object[] parameters) in /_/src/Shared/ObjectMethodExecutor/ObjectMethodExecutor.cs:line 107
 at Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher`1.<ExecuteMethod>d__23[[Microsoft.AspNetCore.SignalR.Tests.HubT, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in /_/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs:line 563
 at Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher`1.<<Invoke>g__ExecuteInvocation|18_0>d[[Microsoft.AspNetCore.SignalR.Tests.HubT, Microsoft.AspNetCore.SignalR.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in /_/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs:line 373
===================
 at Microsoft.AspNetCore.SignalR.Tests.VerifyNoErrorsScope.Dispose() in /home/tester/aspnetcore/src/Shared/SignalR/VerifyNoErrorScope.cs:line 64
at Microsoft.AspNetCore.SignalR.Tests.HubConnectionHandlerTests.HubsCanSendToUser(Type hubType) in /_/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs:line 1930
--- End of stack trace from previous location ---

If I understand #93770 (comment) correctly all these tests were passing with preview6.

We can investigate these further independent of this PR.

lambdageek · 2023-11-07T20:00:57Z

This ArgumentNullException happens in several tests

This code is probably responsible:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

Lines 159 to 165 in 952ac21

    
           // FIXME: GetCustomAttributesBase doesn't like being passed a null attributeType 
        
           if (attributeType == typeof(CustomAttribute)) 
        
               attributeType = null!; 
        
           if (attributeType == typeof(Attribute)) 
        
               attributeType = null!; 
        
           if (attributeType == typeof(object)) 
        
               attributeType = null!;

and apparently RuntimeParameterInfo.GetCustomAttribtue doesn't like that null flowing down. Possibly the fact that AspNetCore has their own subclass of ParameterInfo is confusing the delicate balance that is the mono CustomAttribute spaghetti logic

https://github.com/dotnet/aspnetcore/blob/556f39af49b1a32f14089d3809557825932e4ca5/src/Shared/PropertyAsParameterInfo.cs#L15C1-L15C1

I think this is worth a separate issue.

System.TypeLoadException: VTable setup of type Microsoft.AspNetCore.SignalR.TypedClientBuilder.ITestImpl failed

I'm not getting anywhere with this one... I can't even find ITestImpl anywhere in AspNetCore or the SignalR repo. Is it some kind of generated class with a made-up name?

BrennanConroy · 2023-11-07T20:09:04Z

I can't even find ITestImpl anywhere in AspNetCore or the SignalR repo. Is it some kind of generated class with a made-up name?

Yes. https://github.com/dotnet/aspnetcore/blob/556f39af49b1a32f14089d3809557825932e4ca5/src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs#L49

lambdageek · 2023-11-07T20:45:42Z

Created #94488 for the ArgumentNullException with a standalone repro

lambdageek · 2023-11-07T21:08:22Z

Created #94490 for the VTable fail

…rtual generic methods (#94478) Backport of #94437 to release/6.0-staging Fixes #93770 * [mono] [imt] Don't increment vt_slot for non-virtual generic methods Interfaces can have static generic methods, for example. They don't have a vt_slot. When building an IMT slot, we need to collect all the interface methods that map to a particular IMT slot along with their implementing vtable entries. To do that, vt_slot starts at the interface offset of a particular interface and keeps incrementing as we iterate over the methods of the interface. It is crtitical that vt_slot is accurate - otherwise we may dispatch the interface call to the wrong virtual method. * [mono][imt] remove dead appdomain code the extra_interfaces argument was used to implement additional interfaces on cross-domain transparent proxy objects. * [mono][imt] fixup ifdefed debug code * Add test case

…rtual generic methods (#94468) Backport of #94437 to release/7.0-staging Fixes #93770 * [mono] [imt] Don't increment vt_slot for non-virtual generic methods Interfaces can have static generic methods, for example. They don't have a vt_slot. When building an IMT slot, we need to collect all the interface methods that map to a particular IMT slot along with their implementing vtable entries. To do that, vt_slot starts at the interface offset of a particular interface and keeps incrementing as we iterate over the methods of the interface. It is crtitical that vt_slot is accurate - otherwise we may dispatch the interface call to the wrong virtual method. * [mono][imt] remove dead appdomain code the extra_interfaces argument was used to implement additional interfaces on cross-domain transparent proxy objects. * [mono][imt] fixup ifdefed debug code * Add test case --------- Co-authored-by: Aleksey Kliger <alklig@microsoft.com>

lambdageek added 3 commits November 6, 2023 16:07

[mono][imt] remove dead appdomain code

636aff6

the extra_interfaces argument was used to implement additional interfaces on cross-domain transparent proxy objects.

[mono][imt] fixup ifdefed debug code

16e5ca1

lambdageek requested review from vargaz and thaystg as code owners November 6, 2023 21:09

dotnet-issue-labeler bot added the area-Codegen-JIT-mono label Nov 6, 2023

ghost assigned lambdageek Nov 6, 2023

lambdageek added area-Codegen-meta-mono and removed area-Codegen-JIT-mono labels Nov 6, 2023

lewing mentioned this pull request Nov 6, 2023

Curl fails to download dotnet-install.sh dotnet/dnceng#1372

Closed

3 tasks

vargaz approved these changes Nov 7, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Nov 7, 2023

[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-virtual generic methods #94468

Merged

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

github-actions bot mentioned this pull request Nov 7, 2023

[release/8.0-staging] [mono] [imt] Don't increment vt_slot for non-virtual generic methods #94469

Merged

This comment was marked as outdated.

Sign in to view

Add test case

d39df5d

lambdageek mentioned this pull request Nov 7, 2023

[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-virtual generic methods #94478

Merged

tmds approved these changes Nov 7, 2023

View reviewed changes

lambdageek mentioned this pull request Nov 7, 2023

[mono][reflection] ArgumentNullException calling GetCustomAttributes on user-defined subtype of ParameterInfo #94488

Closed

lambdageek mentioned this pull request Nov 7, 2023

[mono][System.Reflection.Emit] VTable initialization failure for class implementing an interface #94490

Closed

lambdageek merged commit 0fb7b7d into dotnet:main Nov 8, 2023
160 checks passed

gbalykov mentioned this pull request Nov 8, 2023

Fix test returning 100 unconditionally #94531

Merged

ivanpovazan mentioned this pull request Nov 10, 2023

[mono] Fix GetCustomAttributes System.Reflection API with a custom attribute provider #94602

Merged

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437

[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437

lambdageek commented Nov 6, 2023

lambdageek commented Nov 6, 2023 •

edited

Loading

lewing commented Nov 6, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

This comment was marked as outdated.

This comment was marked as resolved.

tmds commented Nov 7, 2023

lambdageek commented Nov 7, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

tmds commented Nov 7, 2023 •

edited

Loading

lambdageek commented Nov 7, 2023

BrennanConroy commented Nov 7, 2023

lambdageek commented Nov 7, 2023

lambdageek commented Nov 7, 2023

[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437

[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437

Conversation

lambdageek commented Nov 6, 2023

lambdageek commented Nov 6, 2023 • edited Loading

lewing commented Nov 6, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

This comment was marked as outdated.

This comment was marked as resolved.

tmds commented Nov 7, 2023

lambdageek commented Nov 7, 2023

This comment was marked as outdated.

lambdageek commented Nov 7, 2023

lambdageek commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

tmds commented Nov 7, 2023 • edited Loading

lambdageek commented Nov 7, 2023

BrennanConroy commented Nov 7, 2023

lambdageek commented Nov 7, 2023

lambdageek commented Nov 7, 2023

lambdageek commented Nov 6, 2023 •

edited

Loading

tmds commented Nov 7, 2023 •

edited

Loading