-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[mono] Fix Type equality comparisons #52791
Conversation
Tagging subscribers to this area: @CoffeeFlux Issue DetailsThe Mono CoreLib currently implements the equality operator for Type This difference causes System.Text.Json.SourceGeneration to fail Fixed by removing the Mono intrinsic and implementing Type equality
|
Could you have a look at if there are any tests that could be enabled with the fix or add a new one? |
All of System.Text.Json.SourceGeneration.Tests fails without the fix (when using the Mono runtime), because the generator programs invoked by the test emit bogus source code that won't even compile. With the fix the tests build correctly and all pass. |
I'm curious about the performance impact without the interpreter intrinsic... cc: @BrzVlad |
I thought there will be simple or more direct test case |
There's also some failures in System.Text.Json.SourceGeneration.UnitTests, which is a bit more direct. But there doesn't seem to be any test that verifies just the properties of Type itself. Should there be one, and where should it live? |
I'm wondering if that's intentional (/cc @layomia @steveharter)
It should go to https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Type |
Looks like there's a failure in the
|
Huh, that's strange. It is not clear to me how this change could cause an LLVM error ... @MichalStrehovsky, I'm now using the implementation from the nativeaot branch -- did you see anything like this error on that branch? I'm not quite sure how to go about debugging this failure without access to an aarch64 machine, unfortunately. Any suggestion on how to extract the failing case? |
Yeah it's weird. Could you try rebasing on main just to make sure? We can figure something out if it still happens. |
Looks like this didn't help, the same LLVM error is still there. |
@lambdageek @CoffeeFlux @imhameed would you mind helping @uweigand to figure out why this change causes a failure in the |
I'll take a look; I have a working environment ready to go on an arm64 server. |
I've submitted a fix for the underlying problem here: #53707 |
This should be fixed. Try rebasing from main. |
The Mono CoreLib currently implements the equality operator for Type using an intrinsic that effectively does ReferenceEquals. However, the CLR CoreLib only does that if both sides are RuntimeType; if not, it uses the Equals predicate. This difference causes System.Text.Json.SourceGeneration to fail when built with the Mono runtime, because it uses a subtype of Type (TypeWrapper) that implements a non-default Equals predicate. The Mono logic will simply ignore that, causing various errors. Fixed by removing the Mono intrinsic and implementing Type equality in managed code in a way that should be equivalent to CoreCLR (as implemented there in RuntimeTypeHandle::TypeEQ).
Indeed, looks like this is fixed now, thanks! The two remaining failures on OSX seem unrelated setup problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still suspect we'll want to fix up the interpreter intrinsic rather than remove it entirely, but for now LGTM.
@BrzVlad thoughts on removed interpreter intrinsic? |
This will have significant performance impact both on jit and interp since the BCL is full of comparisons like |
I noticed there is also a specialization for RuntimeType here: In any case, if you have any suggestions on how to fix the intrinsic implementation so it only catches the valid cases, I'd be happy to implement and test something ... |
It wouldn't help because the result of RyuJIT optimizes this by looking at the tree that feeds the comparison. If either one of the sides of the comparison comes out of |
@uweigand I can implement this intrinsic both on interp and jit but I'm not sure what exact test suite this fixes. For me the json source gen tests crash with and without this change. |
I see the problem just by running the runtime's "./build.sh libs.tests". During compilation, there are a large number of errors (see below) when building To be clear, it doesn't matter if the patch is applied to the runtime sources currently being built; the patch needs to be applied to the host runtime used to do the build itself. Here's the full list of errors I get. They all relate to comparisons between "TypeWrapper" types erroneously being resolved to "false" when they should be "true", which trigger both internal errors in some extension that is being loaded into Roslyn by the generator, as well as generator emitting invalid C# code that causes subsequent compiler errors.
|
Implemented in #54062 |
This does fix the original problem for me, thank you! |
The Mono CoreLib currently implements the equality operator for Type
using an intrinsic that effectively does ReferenceEquals. However,
the CLR CoreLib only does that if both sides are RuntimeType; if not,
it uses the Equals predicate.
This difference causes System.Text.Json.SourceGeneration to fail
when built with the Mono runtime, because it uses a subtype of Type
(TypeWrapper) that implements a non-default Equals predicate. The
Mono logic will simply ignore that, causing various errors.
Fixed by removing the Mono intrinsic and implementing Type equality
in managed code in a way that should be equivalent to CoreCLR (as
implemented there in RuntimeTypeHandle::TypeEQ).