Skip to content

Annotating System.Formats.Asn1 library for Aot#72533

Merged
LakshanF merged 7 commits into
dotnet:mainfrom
LakshanF:AotLibDataFix
Jul 21, 2022
Merged

Annotating System.Formats.Asn1 library for Aot#72533
LakshanF merged 7 commits into
dotnet:mainfrom
LakshanF:AotLibDataFix

Conversation

@LakshanF

@LakshanF LakshanF commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Instead of using Marshal.SizeOf API which causes an AOT warning, wrote a separate implementation as suggested in feedback below to read the size of an enum's underlying type

Enabled the test project to run in AOT rolling run by excluding test scenarios that rely on creating a delegate that has a boxed by-ref parameter, issue #72548 tracks this failure

@ghost

ghost commented Jul 20, 2022

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

ghost commented Jul 20, 2022

Copy link
Copy Markdown

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

Issue Details

Propagated RequiresDynamicCode warning for the Marshal.SizeOf API for the library to be warning free

The generic APIs version of these APIs could be made to use the generic version and be safe but currently they are generating warnings since they go through the non-generic APIs.

Author: LakshanF
Assignees: LakshanF
Labels:

new-api-needs-documentation, area-System.Formats.Asn1, area-NativeAOT-coreclr

Milestone: -

@ghost

ghost commented Jul 20, 2022

Copy link
Copy Markdown

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

Issue Details

Propagated RequiresDynamicCode warning for the Marshal.SizeOf API for the library to be warning free

The generic APIs version of these APIs could be made to use the generic version and be safe but currently they are generating warnings since they go through the non-generic APIs.

Author: LakshanF
Assignees: LakshanF
Labels:

new-api-needs-documentation, area-System.Formats.Asn1, area-NativeAOT-coreclr

Milestone: -

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Marshal.SizeOf API for the library to be warning free

This is easy to change to not use Marshal.SizeOf. For example, define following method somewhere:

static int GetPrimitiveIntegerSize(Type primitiveType)
{
    if (primitiveType == typeof(byte) || primitiveType == typeof(sbyte))
        return 1;
    if (primitiveType == typeof(short) || primitiveType == typeof(ushort))
        return 2;
    if (primitiveType == typeof(int) || primitiveType == typeof(uint))
        return 4;
    if (primitiveType == typeof(long) || primitiveType == typeof(ulong))
        return 8;
    return 0;
}

and use it instead of Marshal.SizeOf.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 20, 2022
@bartonjs

Copy link
Copy Markdown
Member

static int GetPrimitiveIntegerSize(Type primitiveType)

I recommend approximately

ish

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 20, 2022
Comment thread src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj Outdated
Comment thread src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

@LakshanF

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated
Comment thread src/libraries/System.Formats.Asn1/tests/Reader/ReadLength.cs Outdated

@bartonjs bartonjs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rescinding approval based on unresolved feedback in new changes.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 20, 2022
Comment thread src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 20, 2022
@LakshanF

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/libraries/System.Formats.Asn1/tests/Reader/ReadLength.cs Outdated

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM otherwise

@LakshanF

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@LakshanF
LakshanF merged commit cf0f14c into dotnet:main Jul 21, 2022
@LakshanF
LakshanF deleted the AotLibDataFix branch July 21, 2022 15:58
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 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.

5 participants