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

Implement {Try}HashData on asymmetric algorithms #66349

Merged
merged 4 commits into from
Mar 24, 2022

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Mar 8, 2022

An implementation to consider for #66228, if we want to go down this path.

@ghost
Copy link

ghost commented Mar 8, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

An implementation to consider for #66228, if we want to go down this path.

Author: vcsjones
Assignees: vcsjones
Labels:

area-System.Security

Milestone: -

@@ -79,9 +79,9 @@ public static void TestShimProperties()
}

[Fact]
public static void TestShimOverloads()
Copy link
Member Author

@vcsjones vcsjones Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly unrelated from this PR, but the "overloads" in the test names didn't seem correct to me. Consider this a "rider" pull request. 😄

@vcsjones
Copy link
Member Author

vcsjones commented Mar 8, 2022

Open task right now is, for my own education and interest, get a JIT dump for TryHashData to see if some level of tiering burns in the is T check.

@vcsjones
Copy link
Member Author

vcsjones commented Mar 9, 2022

First, a preface: I am by no means experienced with JIT dumps. But I took a stab at looking at a JIT dump of this pattern:

using System;

Console.WriteLine(new FooImpl().Bar());

interface IMarker {}

public abstract class FooBase {
    public virtual int Bar()
    {
        if (this is IMarker)
        {
            Console.WriteLine("marker");
            return 1;
        }
        else
        {
            Console.WriteLine("no marker");
            return 1;
        }
    }
}

public sealed class FooImpl : FooBase, IMarker
{
}

At least on macOS ARM, I don't see the this is IMarker check get elided. It ends up as a helper call to CORINFO_HELP_ISINSTANCEOFINTERFACE.

IN0001: 00000C                    ldr     x1, [fp,#24]
IN0002: 000010                    movz    x0, #0xa600
IN0003: 000014                    movk    x0, #0x80e9 LSL #16
IN0004: 000018                    movk    x0, #2 LSL #32
IN0005: 00001C                    bl      CORINFO_HELP_ISINSTANCEOFINTERFACE
etc

I'm not sure how much we care, but it was a fun exercise.

@bartonjs I am marking this as "ready", but if we want to go back to overrides because we end up with interface checks, or if we can figure out how my testing was wrong, I am happy to adjust.

@vcsjones vcsjones marked this pull request as ready for review March 9, 2022 18:44
@bartonjs bartonjs merged commit 5697987 into dotnet:main Mar 24, 2022
@vcsjones vcsjones deleted the hash-data-cleanup branch March 24, 2022 21:49
@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants