Enable loading composite r2r images from a singlefile bundle

[VSadov]

The main part of the change is making NativeImage::Open to probe for r2r.dll in the bundle and if available load it using the same codepath as for regular r2r assemblies.

Other changes are to make the checks in PE decoder to accept COR header shape is it is produced by crossgen for r2r.dll.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[VSadov]

rebased to pick P4 toolset

[ghost]

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

Issue Details

---

In progress. Needs tests.

Author:	VSadov
Assignees:	-
Labels:	area-Single-File, area-VM-coreclr
Milestone:	-

[VSadov]

@agocke - According to @trylek analysis the reasons why the new test fails are nontrivial and on SDK side and a fix may not be simple and even when fixed we may not be able to rely on the fix in runtime repo for a long time (possibly not before we use preview 7 toolset.
It may be possible to come up with some temporary workaround, but even that may take time.

The suggestion is to accept the change without a test and rely on testing in SDK repo. The integration frequency to SDK is about once a day, which might be sufficient to guard against regressions.

[trylek]

Looks great to me, thank you for fixing composite R2R images in bundled mode!

[agocke]

It looks like the bundler also records the file type of everything in the bundle. Is there any reason why we would want to add a new "R2R composite" file type?

It doesn't really look like we'll need it, as long as we assume that the bundle-relative path is constructed properly at the beginning.

[VSadov]

I do not think we have concrete reasons to distinguish and propagate the distinction for composite r2r files vs. regular r2r assemblies. The main difference is that composite r2r file may contain native code for more than one assembly, which is opaque to the host/bundle. Also it does not seem to contain any IL (I am not sure if it actually can, but it would make no difference either).

As things are now, a separate classification for composite r2r at bundle level would not add much benefit. I think we can wait until/if we have a use case for that.

[agocke]

Change LGTM but it would be good to have the SDK test created and linked here before we merge, to make sure that there's some eventual validation of the change.

[trylek]

Hi Vlad!

I have finally managed to shed some light on the bundled test failures using live SDK bits. I disproved my yesterday theory that the bug was somehow introduced by David Wrighton’s refactoring of composite build in the SDK. After I became able to build the failing tests using a live-built SDK with additional instrumentation, I have found out that there are several bugs involved:

1. The bundled test scripts don’t set the property PublishReadyToRunUseCrossgen2. While runtime framework build was switched over to use CG2 some time ago, in that particular case we added the option PublishReadyToRunUseCrossgen2 to the SDK script invocations. I guess that “SDK switchover to Crossgen2” should involve setting this option to true by default (when PublishReadyToRun is specified) but apparently that isn’t happening even in today SDK upstream/main. I think that’s a bug that should be fixed in Microsoft.NET.Publish.targets.
2. Not setting the property PublishReadyToRunUseCrossgen2 was actually still silently using Crossgen1 under the hood so that fails now that Manish has merged in its removal. We need to fix this option in the host test scripts because it will take a while before the SDK fix will propagate back to the runtime repo.
3. After I fixed the PublishReadyToRunUseCrossgen2 option, I now see a different error – it seems to me that the mock TestPackageCache is missing support for Crossgen2 and subsequently GetPackageDirectory in Microsoft.NET.Build.Tasks.dll is unable to resolve the location of the “Crossgen2 nuget package” (that should be probably the live-built Crossgen2 pack copied under the TestPackageCache folder) and so it doesn’t set Crossgen2Tool.PackagePath that is subsequently used in ResolveReadyToRunCompilers to locate the tool; as a result we end up with an unresolvable relative path “tools\crossgen2.exe” pointing nowhere.
4. In fact, ResolveReadyToRunCompilers looking at “crossgen2.exe” is a bug in itself, we should be using an explicit “dotnet” to launch directly the “crossgen2.dll” as Jan Kotas pointed out, otherwise we risk using an arbitrary ambient .NET host installed on the machine with potentially detrimental effects.

I believe I know how to fix (1) and probably (4). For (2) and (3), these are closely tied to the host test build infrastructure and I’m less familiar with fixing these.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

[trylek]

OK, some slight corrections - (1) is actually fine in current SDK main, I was looking at a slightly older clone of the repo that didn't include the script fix. In light of this I believe the only remaining SDK problem is potentially (4) and it's a small issue. I believe the tests should be fixable by just fixing (3) and possibly (2) depending on when the property was fixed in the SDK repo.

[trylek]

OK, so the commit that added this fix,

dotnet/sdk@d02d7a7

is from 6 days ago. This likely means that it's not part of Preview4 so that the bullet point (2) still holds for now.

[AntonLapounov]

I will add the SDK test. I suggest to go ahead and merge this change.

[VSadov]

I have staged a PR in SDK repo with a simple acceptance test for a singlefile app with a composite R2R - a testcase similar to what we would have here.
dotnet/sdk#18366

I think this responds to all the concerns and I will merge this PR.

[agocke]

LGTM, merge whenever you're ready

[VSadov]

Thanks!!

[VSadov]

I expect that with complete extraction the SDK test will pass even without this change. I am curious if that is indeed the outcome on the SDK side, so have not merged this yet.

[VSadov]

The baseline behavior on SDK side was exactly as expected - composite r2r worked with forced extraction, and failed without extraction.