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

ilasm doesn't preserve "where T : struct", instead produces "where T : ValueType" #45183

Closed
dagood opened this issue Nov 25, 2020 · 5 comments · Fixed by #45307
Closed

ilasm doesn't preserve "where T : struct", instead produces "where T : ValueType" #45183

dagood opened this issue Nov 25, 2020 · 5 comments · Fixed by #45307
Assignees
Milestone

Comments

@dagood
Copy link
Member

dagood commented Nov 25, 2020

Description

I'm observing in source-build that our 5.0.0 ilasm has a regression when given this IL method signature:

.method /*06000757*/ public hidebysig static 
        int32  Compare<valuetype .ctor (System.ValueType/*020000C7*/) T>(
          valuetype System.Nullable`1/*02000089*/<!!T> n1,
          valuetype System.Nullable`1/*02000089*/<!!T> n2) cil managed

Viewing the result in ILSpy shows me these IL results. Red is "bad", with 5.0.0 ilasm. Green is "good", with old 3.1 ilasm.

 	.method public hidebysig static 
-		int32 Compare<(System.ValueType) T> (
+		int32 Compare<valuetype .ctor (System.ValueType) T> (
 			valuetype System.Nullable`1<!!T> n1,
 			valuetype System.Nullable`1<!!T> n2
 		) cil managed 

This issue looks to me like it's blocking the source-build 5.0.0 GA release.

The FSharp compiler seems to rely on the signature being correct, or else the build fails. With the C# decompilation result, it's easier to see why (for me 😄):

-		public static int Compare<T>(T? n1, T? n2) where T : ValueType
+		public static int Compare<T>(T? n1, T? n2) where T : struct

More details and more examples at dotnet/source-build#1920 (comment).

Full original IL file that may be able to repro: https://raw.githubusercontent.com/dotnet/source-build-reference-packages/ddfd9f5918bdaefc0d02db8ea1ae8a8b1ff11b23/src/targetPacks/ILsrc/netstandard.library/2.0.3/build/netstandard2.0/ref/netstandard.il (signature at line 28420)

(Edited to add in context:) Self-contained repro to run on linux-x64: https://github.com/dagood/repro/tree/ilasm-struct-vs-valuetype

@JulieLeeMSFT @briansull can you help with this?

/cc @dleeapho @dseefeld

Regression?

Seems like a regression from 3.1.0-ish ilasm to a 5.0.0 ilasm. This happened when we upgraded from 3.1.0 to 5.0.0 in dotnet/source-build-reference-packages: dotnet/source-build-reference-packages@fe6afa2...c52443b.

The 5.0.0 ilasm includes the recent patch to fix a different regression: #44850.

@briansull
Copy link
Contributor

I will take a look

@briansull briansull self-assigned this Nov 25, 2020
@briansull
Copy link
Contributor

It is helpful to have a complete repro that is ready to try.

@dagood
Copy link
Member Author

dagood commented Nov 25, 2020

Here's a self-contained repro to run on linux-x64: https://github.com/dagood/repro/tree/ilasm-struct-vs-valuetype

@briansull
Copy link
Contributor

I have a fix for this now

@dseefeld
Copy link
Contributor

I added a patch for source-build and tested it locally. It fixes the issue. Patch included in source-build here: dotnet/source-build#1933

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Dec 1, 2020
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Dec 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants