Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve the System.RuntimeType constructor in mono ILLink #48710

Merged
merged 3 commits into from
Feb 25, 2021

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Feb 24, 2021

Fixes #48522

Without this, the linker will think that RuntimeType is never instantiated and can generate code such as (the red without this fix vs the green with this fix):

-      Type underlyingSystemType = type.UnderlyingSystemType;
-      RuntimeType runtimeType = (RuntimeType) null;
-      if ((object) runtimeType != null)
-        return runtimeType.CreateInstanceImpl(bindingAttr, binder, args, culture);
+      RuntimeType underlyingSystemType = type.UnderlyingSystemType as RuntimeType;
+      if ((object) underlyingSystemType != null)
+        return underlyingSystemType.CreateInstanceImpl(bindingAttr, binder, args, culture);

Additionally, under this scenario the runtime will end up failing to load S.P.Corelib with an error similar to:

WASM-ERR: [ERROR] FATAL UNHANDLED EXCEPTION: Nested exception detected.
WASM-ERR: Original Exception: at System.Collections.Generic.Dictionary`2<string, object>..ctor (int,System.Collections.Generic.IEqualityComparer`1<string>) <0x00060>
WASM-ERR: at System.Collections.Generic.Dictionary`2<string, object>..ctor (int) <0x00014>
WASM-ERR: at System.AppContext.Setup (char**,char**,int) <0x00016>
WASM-ERR:
WASM-ERR: Nested exception:at System.RuntimeType.get_BaseType () <0x00056>
WASM-ERR: at System.Type.IsSubclassOf (System.Type) <0x00044>
WASM-ERR: at System.Reflection.CustomAttribute.IsDefined (System.Reflection.ICustomAttributeProvider,System.Type,bool) <0x00040>
WASM-ERR: at System.Reflection.RuntimeMethodInfo.IsDefined (System.Type,bool) <0x00016>
WASM-ERR: at System.Diagnostics.StackTrace.ShowInStackTrace (System.Reflection.MethodBase) <0x00048>
WASM-ERR: at System.Diagnostics.StackTrace.ToString (System.Diagnostics.StackTrace/TraceFormat,System.Text.StringBuilder) <0x000c8>
WASM-ERR: at System.Diagnostics.StackTrace.ToString (System.Diagnostics.StackTrace/TraceFormat) <0x00036>
WASM-ERR: at System.Exception.GetStackTrace (bool) <0x0006a>
WASM-ERR: at System.Exception.get_StackTrace () <0x0000e>
WASM-ERR: at System.Exception.ToString () <0x000ae>
WASM-ERR: at System.Exception.ToString () <0x00054>
WASM-ERR:
WASM-ERR:

@dotnet-issue-labeler
Copy link

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

@tannergooding
Copy link
Member Author

CC. @eerhardt

@eerhardt
Copy link
Member

I think this might fix #48522

@eerhardt eerhardt requested a review from vargaz February 24, 2021 18:19
@joperezr
Copy link
Member

I think this might fix #48522

Confirmed, just ran the tests with this change and without the workaround I added and all test pass for browser-wasm. I'll take the liberty to modify the PR description to add the fixes message.

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down @tannergooding

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm super happy you were able to trace this down. Nice work.

Co-authored-by: Larry Ewing <lewing@microsoft.com>
@ghost
Copy link

ghost commented Feb 24, 2021

Hello @tannergooding!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@joperezr
Copy link
Member

The failure in the dotnet-liner-tests pipeline is a race condition that I'm already fixing on my PR #48429 so feel free to disregard that

@tannergooding tannergooding merged commit 4af65be into dotnet:master Feb 25, 2021
@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Runtime.InteropServices.Javascript is getting trimmed out in some cases for browser-wasm
7 participants