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

System.Reflection.MetadataLoadContext doesn't support function pointer types #43791

Closed
Tracked by #93172
vargaz opened this issue Oct 24, 2020 · 18 comments · Fixed by #81006
Closed
Tracked by #93172

System.Reflection.MetadataLoadContext doesn't support function pointer types #43791

vargaz opened this issue Oct 24, 2020 · 18 comments · Fixed by #81006
Assignees
Labels
area-System.Reflection enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@vargaz
Copy link
Contributor

vargaz commented Oct 24, 2020

To reproduce: run the attached project
Version: master
Expected result: works
Actual result:

Unhandled exception. System.NotSupportedException: Parsing function pointer types in signatures is not supported.
   at System.Reflection.TypeLoading.Ecma.EcmaModule.GetFunctionPointerType(MethodSignature`1 signature)
   at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeType(BlobReader& blobReader, Boolean allowTypeSpecifications, Int32 typeCode)
   at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeMethodSignature(BlobReader& blobReader)
   at System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.SpecializeMethodSig(IRoMethodBase roMethodBase)
   at System.Reflection.TypeLoading.RoDefinitionMethod`1.ComputeMethodSig()
   at System.Reflection.TypeLoading.RoMethod.get_MethodSig()
   at System.Reflection.TypeLoading.RoMethod.get_ReturnParameter()
   at System.Reflection.TypeLoading.RoMethod.get_ReturnType()
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Oct 24, 2020
@vargaz
Copy link
Contributor Author

vargaz commented Oct 24, 2020

test.tar.gz

@marek-safar
Copy link
Contributor

Hmm how is Roslyn reading such metadata?

@jaredpar @MichalStrehovsky

@MichalStrehovsky
Copy link
Member

This looks like a bug in System.Reflection.MetadataLoadContext. It's a library built on top of System.Reflection.Metadata that provides additional abstractions over the metadata. Roslyn only uses S.R.Metadata, not this extra layer with the missing implementation.

@steveharter steveharter added this to the 6.0.0 milestone Nov 11, 2020
@steveharter steveharter added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Nov 11, 2020
@steveharter steveharter self-assigned this Nov 11, 2020
@steveharter
Copy link
Member

steveharter commented Nov 12, 2020

This looks like a bug in System.Reflection.MetadataLoadContext

It is apparently by design since it throws an explicit NotSupportedException when detecting a function pointer.

This overlaps with #11354 where we don't return function pointer type from standard reflection either and need a way to represent that.

Doing a typeof(delegate*<void>) (for example) returns System.IntPtr as the type, so it would be expected that MLC return IntPtr as the type. However, that doesn't allow inspection of the parameters or return type, and there is not an existing "function pointer" type in .NET that we can readily use instead.

@steveharter
Copy link
Member

@vargaz @CoffeeFlux can you add more detail including the workaround you use, and the "blocking" label if you consider it blocking. Thanks

@vargaz
Copy link
Contributor Author

vargaz commented Nov 12, 2020

We just hardcode the method names which have these types in our code, it's not currently a blocker for us.

@lewing
Copy link
Member

lewing commented Nov 13, 2020

@steveharter the current workaround is here

@jeromelaban
Copy link
Contributor

This is a feature that will be a important addition for libraries like SkiaSharp running on WebAssembly.

@lewing
Copy link
Member

lewing commented Nov 28, 2020

Yes it will be required for #44636 if we continue to use MLC

@lewing
Copy link
Member

lewing commented Jul 22, 2021

This will be an ongoing issue for blazor/webassembly.

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 22, 2021
@marek-safar
Copy link
Contributor

@krwq why was it moved to 7.0 when #43791 (comment) explained that this is needed for P1 .NET user story?

@MichalStrehovsky
Copy link
Member

My 5 cents: I'm not sure this can be implemented in MetadataLoadContext without first defining how function pointers are supposed to look like within the runtime/reflection stack (#11354 tracks parts of that), like @steveharter pointed out.

Function pointer behavior is currently pretty arbitrary within the runtime reflection stack: https://twitter.com/MStrehovsky/status/1311247635756412928

I don't know if it's good use of time to make MetadataLoadContext emulate that behavior only so that we can break it again when #11354 is defined just because a tool took a dependency on it.

I would suggest looking into a technology that is more suitable to build compiler components - System.Reflection.Metadata directly, or even Cecil.

@vargaz
Copy link
Contributor Author

vargaz commented Jul 24, 2021

Maybe we can fix our wasm tooling to work around this, but function pointer types are increasing in usage, so other users will run into this problem.

@krwq
Copy link
Member

krwq commented Jul 28, 2021

@marek-safar we've moved most of the issues marked as enhancement to 7.0.0/Future - must've missed the comment. Should this issue have arch-wasm - this way it will disappear from our radar?

@lambdageek
Copy link
Member

@steveharter Will the #69273 MetadataLoadContext work also happen in 7? Will that include also the .NET Framework version of MLC? If so, that will help us with this issue with the wasm tooling.

@steveharter
Copy link
Member

@lambdageek yes MetadataLoadContext will be updated along with the CoreClr runtime. The active PR is #71516.

It should be possible to use .NET framework with MetadataLoadContext function pointers although reflection will necessary to call the new APIs on Type. I'll provide an example once that PR is in.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 6, 2022
@jkotas
Copy link
Member

jkotas commented Jul 11, 2022

It should be possible to use .NET framework with MetadataLoadContext function pointers although reflection will necessary to call the new APIs on Type. I'll provide an example once that PR is in.

We may want to provide extension methods so that it is not necessary to use reflection.

@steveharter
Copy link
Member

Moving to 8.0; we need to update the API and design based on feedback from #71516.

@steveharter steveharter modified the milestones: 7.0.0, 8.0.0 Jul 13, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 24, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection enhancement Product code improvement that does NOT require public API changes/additions
Projects
No open projects