Skip to content

Convert AssemblyChecker to use MetadataLoadContext to avoid file locking. - #133202

Open
jkoritzinsky wants to merge 1 commit into
mainfrom
dev/jkoritzinsky/assemblychecker
Open

Convert AssemblyChecker to use MetadataLoadContext to avoid file locking.#133202
jkoritzinsky wants to merge 1 commit into
mainfrom
dev/jkoritzinsky/assemblychecker

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

The existing implementation was leading to file lock conflicts on #133188 and other managed ILASM PRs.

…ing.

The existing implementation was leading to file lock conflicts on #133188 and other managed ILASM PRs.
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The new MetadataLoadContext resolver strategy is likely to fail on assemblies with non-runtime attribute dependencies, and several thrown exceptions are missing actionable messages.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2 Medium severity · 1 Low severity

New issues introduced by this change (3)
Severity Finding
Medium severity src/​coreclr/​tools/​AssemblyChecker/​AssemblyInspector.cs — MetadataLoadContext resolution is currently limited to runtime assemblies, but IsDebug enumerates…
Medium severity src/​coreclr/​tools/​AssemblyChecker/​AssemblyInspector.cs — RuntimeAssemblyResolver currently relies on typeof(object).Assembly.Location to find the runtime…
Low severity src/​coreclr/​tools/​AssemblyChecker/​AssemblyInspector.cs — Throwing BadImageFormatException without a message makes failures hard to diagnose. Include a brief…
What changed in this PR

This PR updates the CoreCLR AssemblyChecker tool’s --is-debug check by moving the implementation out of Program.cs into a new helper that uses MetadataLoadContext, aiming to avoid persistent file locks caused by Assembly.LoadFrom.

Changes:

  • Removed the in-process Assembly.LoadFrom-based debug check from Program.cs and routed --is-debug through a new helper.
  • Added AssemblyInspector.IsDebug using MetadataLoadContext to inspect DebuggableAttribute.
  • Added a System.Reflection.MetadataLoadContext package reference to support the new implementation.
File Description
src/​coreclr/​tools/​AssemblyChecker/​Program.cs Removes the Assembly.LoadFrom-based IsDebug implementation and forwards --is-debug to AssemblyInspector.
src/​coreclr/​tools/​AssemblyChecker/​AssemblyInspector.cs Adds MetadataLoadContext-based logic and a resolver to determine whether optimizations are disabled via DebuggableAttribute.
src/​coreclr/​tools/​AssemblyChecker/​AssemblyChecker.csproj Adds the System.Reflection.MetadataLoadContext package dependency.
Suppressed comments (2)

src/coreclr/tools/AssemblyChecker/AssemblyInspector.cs:53

  • Throwing BadImageFormatException without a message makes failures hard to diagnose. Include a brief message (and ideally the path) so consumers can tell which assembly/attribute was malformed.
                else
                {
                    throw new BadImageFormatException();
                }

src/coreclr/tools/AssemblyChecker/AssemblyInspector.cs:43

  • Throwing BadImageFormatException without a message makes failures hard to diagnose. Include a brief message (and ideally the path) so consumers can tell which assembly/attribute was malformed.
                {
                    if (arguments[1].Value is not bool optimizationsDisabled)
                    {
                        throw new BadImageFormatException();
                    }

Comment thread src/coreclr/tools/AssemblyChecker/AssemblyInspector.cs
Comment thread src/coreclr/tools/AssemblyChecker/AssemblyInspector.cs
Comment thread src/coreclr/tools/AssemblyChecker/AssemblyInspector.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status
Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants