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

Move Win32 resource emission to the compiler #88464

Merged
merged 9 commits into from
Jul 11, 2023

Conversation

MichalStrehovsky
Copy link
Member

Fixes #79634.

To generate Win32 resources, we'd first use an MSBuild task to dump Win32 resources from the input IL module into a .res file. We'd then pass the .res file to link.exe. Link.exe would then invoke cvtres.exe to convert the .res file to .obj. Link.exe then links the .obj into the final executable.

Skip all of this and instead generate the correct object data in the compiler directly. I'm reusing Win32 resource emission logic from crossgen2. It already mostly does the right thing - we just needed to split the .rsrc section into .rsrc$01 and .rsrc$02 that link.exe expects. The first part has the data directory. The second part has the actual resource data.

Contributes to #73931. The cvtres step was actually non-deterministic because it creates an object file in TEMP and embeds this temporary name into the object.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Jul 6, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #79634.

To generate Win32 resources, we'd first use an MSBuild task to dump Win32 resources from the input IL module into a .res file. We'd then pass the .res file to link.exe. Link.exe would then invoke cvtres.exe to convert the .res file to .obj. Link.exe then links the .obj into the final executable.

Skip all of this and instead generate the correct object data in the compiler directly. I'm reusing Win32 resource emission logic from crossgen2. It already mostly does the right thing - we just needed to split the .rsrc section into .rsrc$01 and .rsrc$02 that link.exe expects. The first part has the data directory. The second part has the actual resource data.

Contributes to #73931. The cvtres step was actually non-deterministic because it creates an object file in TEMP and embeds this temporary name into the object.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

dataBuilder.EmitReloc(node, RelocType.IMAGE_REL_BASED_ADDR32NB, offsetFromSymbol);
dataBuilder.EmitReloc(node,
#if READYTORUN
RelocType.IMAGE_REL_BASED_ADDR32NB,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we have an arbitrary difference in how these RelocTypes are used between crossgen vs. ilc. Which one is right?

(just asking, not something to fix in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

The one crossgen2 is using came from a projectn integration and never meant anything in nativeaot. But it looked like absolute has some meaning to crossgen2 so it's all confusing.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas
Copy link
Member

jkotas commented Jul 6, 2023

Build error:

 Unhandled exception: Internal.TypeSystem.TypeSystemException+FileNotFoundException: Failed to load assembly '--resilient'
     at Internal.TypeSystem.ThrowHelper.ThrowFileNotFoundException(ExceptionStringID, String) + 0x30
     at ILCompiler.CompilerTypeSystemContext.GetModuleForSimpleName(String, Boolean) + 0xca
     at ILCompiler.Program.Run() + 0xd5a

Don't know how to properly condition this to "leaf assemblies".
@MichalStrehovsky MichalStrehovsky merged commit 4e48d2d into dotnet:main Jul 11, 2023
@MichalStrehovsky MichalStrehovsky deleted the resources branch July 11, 2023 08:33
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
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.

Consider getting rid of DumpNativeResources task
2 participants