Skip to content

Start using UnmanagedCallersOnly in CoreLib - #39082

Merged
jkotas merged 1 commit into
dotnet:masterfrom
jkotas:unmanaged-callers-only
Jul 21, 2020
Merged

jkotas merged 1 commit into
dotnet:masterfrom
jkotas:unmanaged-callers-only

Conversation

@jkotas

@jkotas jkotas commented Jul 10, 2020

Copy link
Copy Markdown
Member

No description provided.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

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.

@jkotas

jkotas commented Jul 10, 2020

Copy link
Copy Markdown
Member Author

This is crashing in IL linker with:

 Mono.Linker.LinkerFatalErrorException: ILLinker: error IL1005: .Interop.Kernel32.EnumSystemLocalesEx(method,UInt32,Void*,IntPtr): Error processing method '.Interop.Kernel32.EnumSystemLocalesEx(method,UInt32,Void*,IntPtr)' in assembly 'System.Private.CoreLib.dll'
   ---> System.NullReferenceException: Object reference not set to an instance of an object.
     at Mono.Linker.Steps.MarkStep.ProcessInteropMethod(MethodDefinition method)
     at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     --- End of inner exception stack trace ---
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
     at Mono.Linker.Steps.MarkStep.Process()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)

I have opened dotnet/linker#1345
@marek-safar @AaronRobinsonMSFT

@jkotas
jkotas force-pushed the unmanaged-callers-only branch from 4b6b8ba to 0c3b58a Compare July 10, 2020 16:46

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neat.

@AaronRobinsonMSFT

Copy link
Copy Markdown
Member

@jkotas Is this something we want to run some benchmarks on? At least so we all have a general idea on the benefit of doing this work or at the very least to get a baseline of what we may expect for wins.

@jkotas

jkotas commented Jul 11, 2020

Copy link
Copy Markdown
Member Author

The public entrypoints for what I am touching in this PR are pretty bulky methods. I do not expect to see measurable throughput wins. I expect to see delegate allocation and JITing of the reverse delegate interop interop to disappear with this change. I will check that it is what is happening once it actually builds.

My primary motivation for this change was to validate that the feature actually works end-to-end and shake stress bugs if there are any

@marek-safar marek-safar reopened this Jul 15, 2020
@jkotas
jkotas force-pushed the unmanaged-callers-only branch from 0c3b58a to 1bc958e Compare July 15, 2020 14:22
@jkotas

jkotas commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

@vargaz UnmanagedCallersOnly function pointers seems to be broken on Mono. Could you please take look?

Also, PInvokeTable generator has problems with this change, but I should be able to fix that.

@jkotas
jkotas force-pushed the unmanaged-callers-only branch 2 times, most recently from c00f307 to 692ea29 Compare July 15, 2020 18:39
@jkotas

jkotas commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

I have reverted the Icu change for now to see whether there is anything else that breaks. (The full change is at https://github.com/jkotas/runtime/tree/unmanaged-callers-only-full.)

@jkotas

jkotas commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

I expect to see delegate allocation and JITing of the reverse delegate interop interop to disappear with this change

I have verified that this does actually happen.

@jkotas jkotas changed the title WIP: Start using UnmanagedCallersOnly in CoreLib Start using UnmanagedCallersOnly in CoreLib Jul 15, 2020
@jkotas
jkotas requested a review from AaronRobinsonMSFT July 15, 2020 21:28
Comment thread tools-local/tasks/mobile.tasks/WasmAppBuilder/PInvokeTableGenerator.cs Outdated
@jkotas

jkotas commented Jul 16, 2020

Copy link
Copy Markdown
Member Author

x86 failing with:

System.Globalization.Tests.DateTimeFormatInfoLongTimePattern.LongTimePattern_CheckReadingTimeFormatWithSingleQuotes_ICU [SKIP]
      Condition(s) not met: "IsIcuGlobalization"

Assert failure(PID 5328 [0x000014d0], Thread: 4784 [0x12b0]): !pMethod->HasUnmanagedCallersOnlyAttribute()

@vargaz

vargaz commented Jul 16, 2020

Copy link
Copy Markdown
Contributor

How does UnmanagedCallersOnly fail on mono ? Does it fail on wasm or on desktop ?

@am11

am11 commented Jul 16, 2020

Copy link
Copy Markdown
Member

@vargaz, I think it failed on arm64 (desktop). Here are the job runs with ICU interop change. It was this change delegate* <char*, IntPtr, void> callback, which is now reverted. example log:

  • Assertion at /__w/1/s/src/mono/mono/metadata/class-init.c:2241, condition `klass->instance_size == instance_size' not met

@jkotas

jkotas commented Jul 16, 2020

Copy link
Copy Markdown
Member Author

Yes, this was the failure. You can reproduce it on Linux by checking out https://github.com/jkotas/runtime/tree/unmanaged-callers-only-full and running globalization tests.

@vargaz

vargaz commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

Should be fixed by #39517
.

@jkotas

jkotas commented Jul 17, 2020 •

Copy link
Copy Markdown
Member Author

Thanks. Does #39517 fix it on wasm as well?

@jkotas
jkotas force-pushed the unmanaged-callers-only branch from bb39901 to 6dec18b Compare July 17, 2020 23:21
@jkotas

jkotas commented Jul 18, 2020

Copy link
Copy Markdown
Member Author

The new Mono failure pattern is:

Unhandled Exception:\nSystem.InvalidProgramException: Invalid IL due to: method System.Globalization.CalendarData:EnumCalendarInfoCallback (char*,intptr) with UnmanagedCallersOnlyAttribute has non-blittable parameters or return type 

@jkotas
jkotas force-pushed the unmanaged-callers-only branch from 6dec18b to 7a9ccd1 Compare July 18, 2020 04:39
@vargaz

vargaz commented Jul 18, 2020

Copy link
Copy Markdown
Contributor

Can't reproduce that failure.

@jkotas

jkotas commented Jul 19, 2020

Copy link
Copy Markdown
Member Author

CoreCLR test failure is:

  Starting:    Microsoft.Extensions.Logging.Console.Tests (parallel test collections = on, max threads = 4)

Assert failure(PID 22 [0x00000016], Thread: 40 [0x0028]): m_callCountingInfoByCodeVersionHash.Lookup(codeVersion) == nullptr
    File: /__w/1/s/src/coreclr/src/vm/callcounting.cpp Line: 477
    Image: /root/helix/work/correlation/dotnet

@jkotas

jkotas commented Jul 19, 2020

Copy link
Copy Markdown
Member Author

Blocked on #39604

@jkotas jkotas added the blocked Issue/PR is blocked on something - see comments label Jul 19, 2020
- Fix CoreCLR assert on x86
- Use workaround for Mono
@jkotas
jkotas force-pushed the unmanaged-callers-only branch from 7a9ccd1 to b57f39b Compare July 20, 2020 22:04
@jkotas jkotas removed the blocked Issue/PR is blocked on something - see comments label Jul 21, 2020
@jkotas

jkotas commented Jul 21, 2020

Copy link
Copy Markdown
Member Author

All failures are due to #39082

@jkotas
jkotas merged commit 0df6dac into dotnet:master Jul 21, 2020
@jkotas
jkotas deleted the unmanaged-callers-only branch July 21, 2020 02:32
@jkotas

jkotas commented Jul 21, 2020 •

Copy link
Copy Markdown
Member Author

Unhandled Exception:\nSystem.InvalidProgramException: Invalid IL due to: method System.Globalization.CalendarData:EnumCalendarInfoCallback (char*,intptr) with UnmanagedCallersOnlyAttribute has non-blittable parameters or return type

I have extended the Wasm workaround to apply to all Mono targets to work around this. @vargaz You should be able to reproduce this by changing #if MONO back to #if TARGETS_WASM.

@vargaz

vargaz commented Jul 21, 2020

Copy link
Copy Markdown
Contributor

Can't reproduce the mono failures.

@jkotas

jkotas commented Jul 21, 2020

Copy link
Copy Markdown
Member Author

I have submitted #39695 . It should reproduce the failures.

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
- Fix CoreCLR assert on x86
- Use workaround for Mono
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants