Compile System.Private.CoreLib with Crossgen2 by default, take 2

[trylek]

After Bruce's and JanK's fixes I'm no longer able to repro the
previously observed x86 failures in libraries tests locally so I'm
switching back to lab testing; I plan to run several rounds of
PR and outerloop tests to increase my confidence in the change.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

[mangod9]

Interesting that the failure doesnt repro anymore. Could you please link the PRs for the fixes to this PR if they are available?

[trylek]

JanK's fix: #45999
Bruce's fix: #45947

[mangod9]

Thanks for adding those, so now the only criteria is to have CI pass outerloop jobs for x86?

[trylek]

Hmm, the non-deterministic nature of the bug makes it hard to define a precise criterion. On top of that, based on looking at the legs, I suspect that outerloop testing actually adds very little to libraries testing I'm most interested in at this moment. I guess that I'll just rerun the PR something like 5~10 times and I'll keep my fingers crossed - merging this in the next couple of days has the "thanksgiving advantage" that any irregularities will be easily seen in the reduced traffic of the holiday period. Having said that, the current working theory is that CG2 wasn't at fault at all by itself, it's just that some of the subtle diffs in code placement and import cell management aggravated the pre-existing GC hole and tailcall optimization bug that Bruce and JanK subsequently fixed.

[jkotas]

Both crossgen1 and JIT are bailing out for PInvokes from getMethodInfo as well here:

runtime/src/coreclr/vm/jitinterface.cpp

Line 7783 in 891e8d4

if (!ftn->IsDynamicMethod() && (!ftn->IsIL() || !ftn->GetRVA() || ftn->IsWrapperStub()))

Why is this change necessary?

[jkotas]

This change does not appear to be fixing the x86 crash based on my quick local test. Were you able to confirm locally that it is fixing the problem?

[trylek]

While I believe I see a CG1 vs. CG2 diff in NgenDump I tracked down to this difference, I have rolled back the 2nd commit based on your clarification that the error I've been attempting to fix is unrelated to my change. Amusingly this is about the 2nd or 3rd time the SPC CG2 switchover revealed a bug that ended up being completely unrelated to the switchover as such. Let's hope it just means that we do take CG2 testing seriously ;-).

[jkotas]

While I believe I see a CG1 vs. CG2 diff in NgenDump I tracked down to this difference,

Do you still see the diff in the current master? If yes, could you please share the details of the diff?

[trylek]

I have removed the WIP status as the only remaining bug revealed by repeated testing of this change turns out to be unrelated to it - it's apparently #46184 based on JanK's clarification. I'll run another round of outerloop testing but right now I'm not aware of any remaining issues actually caused by this switch-over so I believe it's ready to be merged in.

[trylek]

Superseded by #47019, closing.