Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Validate MVIDs of assemblies in large version bubble#25281

Closed
MichalStrehovsky wants to merge 10 commits into
dotnet:masterfrom
MichalStrehovsky:validVersionBubble
Closed

Validate MVIDs of assemblies in large version bubble#25281
MichalStrehovsky wants to merge 10 commits into
dotnet:masterfrom
MichalStrehovsky:validVersionBubble

Conversation

@MichalStrehovsky
Copy link
Copy Markdown
Member

This reuses the logic we had lying around for fragile NGen.

We store the pointer to the data in the ready to run header instead.

This reuses the logic we had lying around for fragile NGen.

We store the pointer to the data in the ready to run header instead.
Comment thread src/zap/zapimage.cpp Outdated
@MichalStrehovsky
Copy link
Copy Markdown
Member Author

@janvorli @trylek Could you have a look?

Cc @sergiy-k

Comment thread src/inc/pedecoder.inl Outdated
Comment thread src/inc/pedecoder.h Outdated
Comment thread src/inc/pedecoder.inl Outdated
@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review June 24, 2019 13:02
Comment thread src/inc/pedecoder.h Outdated
Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
@MichalStrehovsky
Copy link
Copy Markdown
Member Author

Well of course there's Unix specific test failures. Looking...

+ /Users/dotnet-bot/dotnetbuild/work/799a5e96-6449-48cc-9f27-1b2f7ba70c5a/Payload/corerun /Users/dotnet-bot/dotnetbuild/work/799a5e96-6449-48cc-9f27-1b2f7ba70c5a/Payload/xunit.console.dll baseservices/threading/baseservices.threading.XUnitWrapper.dll -parallel collections -nocolor -noshadow -xml testResults.xml -trait TestGroup=baseservices.threading

Assert failure(PID 90044 [0x00015fbc], Thread: 1510854 [0x170dc6]): m_pLayouts[IMAGE_LOADED]!=NULL
    File: /Users/vsts/agent/2.153.2/work/1/s/src/vm/peimage.inl Line: 91
    Image: /Users/dotnet-bot/dotnetbuild/work/799a5e96-6449-48cc-9f27-1b2f7ba70c5a/Payload/corerun

Copy link
Copy Markdown
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Please look into what happens with AssemblyLoadContexts. As this feature is somewhat experimental, I'm ok with what I believe are fail-safe modes, but it will prevent full utilization of the feature for some customers.

Comment thread src/vm/domainfile.cpp Outdated
pImage->GetNativeMDImport(),
GetDomainAssembly());

GetAppDomain()->CheckForMismatchedNativeImages(&name, &pDependency->Mvid);
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.

The CheckForMismatchedNativeImages feature doesn't appear to me to be correct in the presence of AssemblyLoadContext's with non-default behavior which load ReadyToRun images.

There are 2 issues

  1. The list is AppDomain level, and that's not appropriate for checks that do not have AppDomain locality.
  2. There is assembly name normalization that occurs as part of CheckForMismatchedNative images. This normalization is appropriate for the non-AssemblyLoadContext world, but in the presence of assembly load contexts, normalization isn't correct.

@MichalStrehovsky
Copy link
Copy Markdown
Member Author

The newly added test is failing because we're apparently not picking up the .ni.exe files.

After trying to find the thing I broke for half a day today I found out that with vanilla master CoreCLR:

echo class Program { static int Main() =^> 42; } > test.cs
csc /noconfig /nostdlib test.cs /r:System.Private.CoreLib.dll
crossgen /p . test.exe
set complus_jitdisasm=Main
corerun test.exe
[prints nothing]
move test.* ..\
corerun ..\test.exe
[lots of spew because we're jitting]

Is it expected that we're not loading .ni.exe files unless they're in CORE_ROOT?

If I go and set complus_jitdisasm=TestStaticFields before running the existing readytorun\tests\mainv1 I see it doing jitting too despite the test trying to crossgen itself.

@davidwrighton
Copy link
Copy Markdown
Member

That doesn't sound expected to me.

@MichalStrehovsky
Copy link
Copy Markdown
Member Author

That doesn't sound expected to me.

@davidwrighton I have a fix out in #25592.

@maryamariyan
Copy link
Copy Markdown

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

@sergiy-k sergiy-k added the post-consolidation PRs which will be hand ported to dotnet/runtime label Nov 7, 2019
@maryamariyan
Copy link
Copy Markdown

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ReadyToRun post-consolidation PRs which will be hand ported to dotnet/runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants