Skip to content

Implement NoSEH flag setting for crossgened modules in ILLink #117779

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

Merged

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 17, 2025

This PR implements the product fix to ensure the NoSEH flag is correctly set in the PE header when ILLink processes crossgened assemblies.

Fixes #117752

Changes Made

Product Fix:

  • Updated WriteAssembly() method in OutputStep.cs to set module.Characteristics |= ModuleCharacteristics.NoSEH when processing crossgened modules
  • The NoSEH flag is now set alongside existing IL-only attributes for crossgened assemblies

Technical Details

The implementation ensures that when a module is detected as crossgened (via module.IsCrossgened()), the NoSEH characteristic is added to indicate that the binary does not use Structured Exception Handling, which is important for proper PE header compliance.

This addresses issue #117752 by providing the actual implementation to ensure ILLink maintains proper PE header characteristics for crossgened modules.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@agocke
Copy link
Member

agocke commented Jul 17, 2025

Fixes #117752

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 17, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 17, 2025
@Copilot Copilot AI changed the title [WIP] Add unit test to verify NoSEH bit is set on R2R-stripped output assemblies Add NoSEH flag verification test for ILLink PE header processing Jul 17, 2025
@Copilot Copilot AI requested a review from agocke July 17, 2025 18:20
Copilot finished work on behalf of agocke July 17, 2025 18:20
@agocke
Copy link
Member

agocke commented Jul 17, 2025

Please fix the compiler errors in the dotnet-linker-tests due to ambiguous namespaces

Remove broad using statements for System.Reflection.Metadata and System.Reflection.PortableExecutable namespaces that were causing ambiguity with existing Individual test namespaces. Fully qualify the PE types to avoid conflicts.

Co-authored-by: agocke <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jul 17, 2025

Please fix the compiler errors in the dotnet-linker-tests due to ambiguous namespaces

Fixed the ambiguous namespace compiler errors by removing the broad using statements for System.Reflection.Metadata and System.Reflection.PortableExecutable that were conflicting with the existing Individual test namespaces. The PE types are now fully qualified in the test method to avoid ambiguity. (976e823)

@AaronRobinsonMSFT
Copy link
Member

@copilot in OutputStep.cs, there is a function called WriteAssembly(). Please update it so that when the module is detected to be crossgened, it does the following module.Characteristics |= ModuleCharacteristics.NoSEH.

Copy link
Contributor Author

Copilot AI commented Aug 6, 2025

@copilot in OutputStep.cs, there is a function called WriteAssembly(). Please update it so that when the module is detected to be crossgened, it does the following `module.Characteristics |= ModuleC...

Updated the WriteAssembly() method in OutputStep.cs to set module.Characteristics |= ModuleCharacteristics.NoSEH when a module is detected to be crossgened. The change is added alongside the existing attributes modification in the crossgened detection block. (8d3a5d8)

@Copilot Copilot AI changed the title Add NoSEH flag verification test for ILLink PE header processing Add NoSEH flag verification test and implement flag setting for crossgened modules in ILLink Aug 6, 2025
@Copilot Copilot AI requested a review from AaronRobinsonMSFT August 6, 2025 02:21
Copilot finished work on behalf of AaronRobinsonMSFT August 6, 2025 02:21
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Aug 6, 2025
Co-authored-by: AaronRobinsonMSFT <[email protected]>
@Copilot Copilot AI changed the title Add NoSEH flag verification test and implement flag setting for crossgened modules in ILLink Implement NoSEH flag setting for crossgened modules in ILLink Aug 6, 2025
Copilot finished work on behalf of AaronRobinsonMSFT August 6, 2025 14:51
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review August 6, 2025 18:29
@Copilot Copilot AI review requested due to automatic review settings August 6, 2025 18:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a fix to ensure the NoSEH (No Structured Exception Handling) flag is properly set in PE headers when ILLink processes crossgened assemblies. The change addresses issue #117752 by maintaining proper PE header compliance for these modules.

Key changes:

  • Added NoSEH characteristic flag setting for crossgened modules in ILLink's output step
  • Ensures PE header compliance alongside existing IL-only attributes

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit a3df2a8 into main Aug 6, 2025
67 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the copilot/fix-72f82504-b3b8-4e9e-a4c1-ff1bbcb9b5a2 branch August 6, 2025 18:31
@agocke
Copy link
Member

agocke commented Aug 7, 2025

Thanks for pushing this along. I was trying to add a test case, but it was a pain to get R2R to run during ILLink testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binskim is reporting BA2018 for trimmed netcoreapp assemblies
4 participants