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

Merged
lambdageek merged 4 commits into
dotnet:mainfrom
lambdageek:fix-gh-93770
Nov 8, 2023
Merged

[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom
lambdageek:fix-gh-93770

Conversation

@lambdageek

@lambdageeklambdageek commented Nov 6, 2023

Copy link
Copy Markdown
Member

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.

@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost assigned lambdageekNov 6, 2023
@lambdageek

lambdageek commented Nov 6, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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)) {
intslot=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

lewing commented Nov 6, 2023

Copy link
Copy Markdown
Member

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'

@lewinglewing mentioned this pull request Nov 6, 2023
Closed
3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-staging

@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0-staging

@github-actions

github-actionsBot commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actionsBot commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-staging

@github-actions

github-actionsBot commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Nov 7, 2023
Merged
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot mentioned this pull request Nov 7, 2023
Merged
@tmds

tmds commented Nov 7, 2023

Copy link
Copy Markdown
Member

@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

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

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

This comment was marked as outdated.

Sign in to view
@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-staging

@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-staging

@github-actions

github-actionsBot commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actionsBot commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

@lambdageeklambdageek mentioned this pull request Nov 7, 2023
Merged
@tmds

tmds commented Nov 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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.

tmds
tmds approved these changes Nov 7, 2023
View reviewed changes
@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

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

BrennanConroy commented Nov 7, 2023

Copy link
Copy Markdown
Member

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

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

Created #94488 for the ArgumentNullException with a standalone repro

@lambdageeklambdageek mentioned this pull request Nov 7, 2023
Closed
@lambdageek

lambdageek commented Nov 7, 2023

Copy link
Copy Markdown
MemberAuthor

Created #94490 for the VTable fail

@lambdageeklambdageek mentioned this pull request Nov 7, 2023
Closed
@lambdageek
lambdageek merged commit 0fb7b7d into dotnet:mainNov 8, 2023
@gbalykovgbalykov mentioned this pull request Nov 8, 2023
Merged
@ivanpovazanivanpovazan mentioned this pull request Nov 10, 2023
Merged
lambdageek added a commit that referenced this pull request Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit that referenced this pull request Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>
This was referenced Nov 14, 2023
Closed
Closed
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
2 more reviewers
@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture

5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[mono] [imt] Don't increment vt_slot for non-virtual generic methods  - #94437
Merged
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Nov 8, 2023
Merged
[mono] [imt] Don't increment vt_slot for non-virtual generic methods #94437
lambdageek merged 4 commits into
dotnet:mainfrom 
lambdageek:fix-gh-93770
Conversation
@lambdageek
@lambdageeklambdageek
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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.
@lambdageek
[mono] [imt] Don't increment vt_slot for non-virtual generic methods
7724033
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.
@lambdageek
[mono][imt] remove dead appdomain code
636aff6
the extra_interfaces argument was used to implement additional
interfaces on cross-domain transparent proxy objects.
@lambdageek
[mono][imt] fixup ifdefed debug code
16e5ca1
@ghostghost
assigned lambdageekNov 6, 2023
@lambdageek
lambdageek
commented
Nov 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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)) { 
intslot=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
lewing
commented
Nov 6, 2023

Copy link


Copy Markdown

Member
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'

@lewinglewing
mentioned this pull request
Nov 6, 2023

Closed

3 tasks
@lambdageek

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/6.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6786091607
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6786093881
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6786100723
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actionsgithub-actionsBot
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
@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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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

This comment was marked as outdated.

Sign in to view
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6788024658
@github-actions
github-actionsBot
commented
Nov 7, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6788026251
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023
 Merged

@tmds
tmds
commented
Nov 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
tmds
tmds
approved these changes
Nov 7, 2023
View reviewed changes
@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
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
BrennanConroy
commented
Nov 7, 2023

Copy link


Copy Markdown

Member
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
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94488 for the ArgumentNullException with a standalone repro
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
commented
Nov 7, 2023

Copy link


Copy Markdown

MemberAuthor
Created #94490 for the VTable fail
@lambdageeklambdageek
mentioned this pull request
Nov 7, 2023

Closed

@lambdageek
lambdageek
merged commit 0fb7b7d
into
dotnet:mainNov 8, 2023
@gbalykovgbalykov
mentioned this pull request
Nov 8, 2023
 Merged

@ivanpovazanivanpovazan
mentioned this pull request
Nov 10, 2023
 Merged

lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@lambdageek
[release/6.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
6f3f5dc
…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
lambdageek added a commit
that referenced
this pull request
Nov 13, 2023
@github-actions@lambdageek
[release/7.0-staging] [mono] [imt] Don't increment vt_slot for non-vi…
87997db
…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>

This was referenced Nov 14, 2023
 Closed

 Closed

 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 9, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

2 more reviewers

@vargazvargazvargaz approved these changes
@tmdstmdstmds approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lambdageeklambdageek

Labels

area-Codegen-meta-mono

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

aspnetcore tests having Dynamic.Proxy fails on ppc64le architecture


5 participants

@lambdageek@lewing@tmds@BrennanConroy@vargaz