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

Add debug-only use of new AssemblyBuilder.Save in Regex.CompileToAssembly #96462

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

stephentoub
Copy link
Member

To aid in debugging RegexCompiler issues and to help vet the new AssemblyBuilder.Save support.

This brings back and then tweaks code that was removed in #59734.

@ghost
Copy link

ghost commented Jan 3, 2024

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

Issue Details

To aid in debugging RegexCompiler issues and to help vet the new AssemblyBuilder.Save support.

This brings back and then tweaks code that was removed in #59734.

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

@stephentoub
Copy link
Member Author

@buyaa-n, the build is failing with:

/_/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs(190,9): Trim analysis error IL2121: System.Reflection.Emit.ModuleBuilderImpl.WriteInterfaceImplementations(TypeBuilderImpl, TypeDefinitionHandle): Unused 'UnconditionalSuppressMessageAttribute' for warning 'IL2072'. Consider removing the unused warning suppression. [D:\a\_work\1\s\src\libraries\sfx.proj]
/_/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs(182,9): Trim analysis error IL2121: System.Reflection.Emit.TypeBuilderImpl.CheckInterfaces(Type[]): Unused 'UnconditionalSuppressMessageAttribute' for warning 'IL2075'. Consider removing the unused warning suppression. [D:\a\_work\1\s\src\libraries\sfx.proj]

Have you seen these?

@stephentoub stephentoub force-pushed the regexassemblysave branch 2 times, most recently from 8ce3b15 to 9b98a60 Compare January 3, 2024 23:16
@stephentoub
Copy link
Member Author

@buyaa-n, I fixed the invalid code gen, but now I'm hitting this:

Unhandled exception. System.MissingMethodException: Method not found: 'System.ReadOnlySpan`1<Char> System.ReadOnlySpan`1.Slice(Int32)'

Is there something related to generics that's not yet implemented maybe?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 3, 2024

@buyaa-n, the build is failing with:

/_/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs(190,9): Trim analysis error IL2121: System.Reflection.Emit.ModuleBuilderImpl.WriteInterfaceImplementations(TypeBuilderImpl, TypeDefinitionHandle): Unused 'UnconditionalSuppressMessageAttribute' for warning 'IL2072'. Consider removing the unused warning suppression. [D:\a\_work\1\s\src\libraries\sfx.proj]
/_/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs(182,9): Trim analysis error IL2121: System.Reflection.Emit.TypeBuilderImpl.CheckInterfaces(Type[]): Unused 'UnconditionalSuppressMessageAttribute' for warning 'IL2075'. Consider removing the unused warning suppression. [D:\a\_work\1\s\src\libraries\sfx.proj]

Have you seen these?

No, never seen those and removing those suppressions locally causing still build failure:

...\src\System\Reflection\Emit\TypeBuilderImpl.cs(184,49): error IL2075: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Collections.IEnumerator.Current.get' does not have matching annotations.
 The source value must declare at least the same requirements as those declared on the target location it is assigned to.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 3, 2024

Is there something related to generics that's not yet implemented maybe?

No generics should have covered completely, no known issue (yet), wonder how that happen, more logs could be helpful

@stephentoub
Copy link
Member Author

@sbomer, can you help @buyaa-n with these illinker warnings?

@stephentoub
Copy link
Member Author

I opened #96469 for the span issue.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 4, 2024

No, never seen those warnings

Now I do see the warnings with your branch, but weirdly removing those suppressions reintroduces the IL2075 warning back (not the IL2072)

...\src\System\Reflection\Emit\TypeBuilderImpl.cs(184,49): error IL2075: 'this' argument does not satisfy 'DynamicallyAcc
essedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Col
lections.IEnumerator.Current.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location
it is assigned to.

I opened #96469 for the span issue.

Thanks, I will fix it ASAP

…mbly

To aid in debugging RegexCompiler issues and to help vet the new AssemblyBuilder.Save support.
@stephentoub
Copy link
Member Author

@sbomer investigated the linker issue and narrowed it down to #96646. He's working on a fix.

@stephentoub
Copy link
Member Author

@sbomer
Fix IL2121 warnings

Thanks.

@stephentoub
Copy link
Member Author

@buyaa-n, can you take another look?

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks for using new AssemblyBuilder implementation.

@stephentoub stephentoub merged commit b4ec422 into dotnet:main Jan 9, 2024
108 of 111 checks passed
@stephentoub stephentoub deleted the regexassemblysave branch January 9, 2024 02:49
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
…mbly (dotnet#96462)

* Add debug-only use of new AssemblyBuilder.Save in Regex.CompileToAssembly

To aid in debugging RegexCompiler issues and to help vet the new AssemblyBuilder.Save support.

* Fix IL2121 warnings

---------

Co-authored-by: Sven Boemer <sbomer@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2024
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.

3 participants