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

Incorrect trimming of static properties when DIM is involved #2865

Closed
dakersnar opened this issue Jun 24, 2022 · 3 comments · Fixed by #2868
Closed

Incorrect trimming of static properties when DIM is involved #2865

dakersnar opened this issue Jun 24, 2022 · 3 comments · Fixed by #2868
Assignees
Milestone

Comments

@dakersnar
Copy link

I'm attempting to add a static virtual DIM (AllBitsSet) to an interface (IBinaryNumber), and then override it in a child (BigInteger). However, the test is failing because the DIM is being called instead of the child's implementation of it.

Here is the commit for reproduction purposes:
dakersnar/runtime@4af1fa8

Reproduction steps:

  • clone the above branch
  • ./build.cmd clr+libs -rc Release
  • cd .\src\libraries\System.Runtime.Numerics\tests
  • dotnet build /t:Test /p:XunitMethodName=System.Numerics.Tests.BigIntegerTests_GenericMath.AllBitsSetTest

Result:

System.Numerics.Tests.BigIntegerTests_GenericMath.AllBitsSetTest [FAIL]
Assert.Equal() Failure
Expected: -1 (this is what BigInteger.AllBitsSet should return
Actual: 1 (this is what the IBinaryNumber DIM returns)

More info

if you observe .\artifacts\obj\System.Runtime.Numerics\Release\net7.0\System.Runtime.Numerics.dll
vs .\artifacts\obj\System.Runtime.Numerics\Release\net7.0\PreTrim\System.Runtime.Numerics.dll
in a tool like ildasm, you'll see that the former has AllBitsSet and the latter is missing it. I suspect this is a trimmer bug.

It seems like similar issues have been looked at already (#2058) (#2711). Given that properties seem to still be buggy, @tannergooding recommends testing properties, events, and other member types explicitly.

@tannergooding
Copy link
Member

CC. @vitek-karas, @agocke

@agocke
Copy link
Member

agocke commented Jun 25, 2022

@jtschuster

@agocke
Copy link
Member

agocke commented Jul 27, 2022

I believe this is fixed.

@agocke agocke closed this as completed Jul 27, 2022
jtschuster added a commit that referenced this issue Aug 3, 2022
…ace methods (#2868)

Fixes #2865
Also addresses marking of all static interface methods encompassing the changes from #2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated.

Co-authored-by: Sven Boemer <sbomer@gmail.com>
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
…ace methods (dotnet/linker#2868)

Fixes dotnet/linker#2865
Also addresses marking of all static interface methods encompassing the changes from dotnet/linker#2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated.

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Commit migrated from dotnet/linker@118bdca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants