Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

Delete unnecessary MarshalImpl abstraction #5404

Merged
merged 1 commit into from
Feb 22, 2018
Merged

Delete unnecessary MarshalImpl abstraction #5404

merged 1 commit into from
Feb 22, 2018

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Feb 18, 2018

MarshalImpl is an extra layer without clear purpose. Also made MarshalAdapter to be included in CoreCLR/Mono builds only.

MarshalImpl is an extra layer without clear purpose. Also made MarshalAdapter to be included in CoreCLR/Mono builds only.
@jkotas jkotas requested a review from luqunl February 18, 2018 23:38
@jkotas jkotas merged commit 9f7f0f7 into dotnet:master Feb 22, 2018
@jkotas jkotas deleted the MarshalImpl branch February 22, 2018 00:50
@MichalStrehovsky
Copy link
Member

This broke Project N build because MCGConsole (the CoreCLR version of MCG) is misusing the System.Private.ILToolchain contract (and the associated WellKnownTypes class) to access the MarshalAdapter class (even though the class is definitely not part of the Project N ILToolchain contract).

The build fails generating the facade with MarshalAdapter removed.

We could fix this by either making MCGConsole stop using WellKnownTypes for this (not sure what's the next best mechanism though), or putting the useless type back into S.P.Interop. Longer term, MCGConsole should probably stop doing this though.

@luqunl Do you have any suggestions?

@jkotas
Copy link
Member Author

jkotas commented Feb 27, 2018

I think just put the useless type back to S.P.Interop.

@luqunl
Copy link
Contributor

luqunl commented Feb 27, 2018

In short, We need MarshalAdapter class to help us to solve Marshal API problem for MCG on CoreCLR.

Currently, there are 3 kinds APIs in interop area:

  • Marshal API: public API, used by customer and exposed in corelib or System.Private.Interop.
  • McgMarshal API: internal use only API, MCG generated code will call McgMarshal API
  • MarshalAdapter API: Use by McgOnCoreCLR which will call McgMarshal API first and if McgMarshal fails, calls Marshal API

Why We added MarshalAdatper API:
Suppose Customer calls some Marshal APIs directly, These Marshal API would call directly into runtime(coreclr.dll) and may fail if these API related to COM or even Delegate, since runtime(coreclr.dll) doesn't know MCG generated data/System.Private.Interop. So what We need is to replace these Marshal APIs into another APIs(such as MarshalAdapter) which will know MCG generated data. The reason why We add a new APIs instead of using McgMarshal API is because of delegate---MCG on CoreCLR Analyzers doesn't analyze call graph, so it doesn't know which delegate type passed on Marshal.GetFunctionPointerForDelegate unless the delegate is marked with UnmanagedFunctionPointerAttribute(it means MCG on CoreCLR won't able to collect all of these delegate type and generate marshaling code). so for Marshal.GetFunctionPointerForDelegate API, We need to check MCG generated data first(if Delegate contains COM interface arguments), if it fails, then call directly into runtime(coreclr.dll)

kbaladurin pushed a commit to kbaladurin/corert that referenced this pull request Mar 15, 2018
MarshalImpl is an extra layer without clear purpose. Also made MarshalAdapter to be included in CoreCLR/Mono builds only.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants