Fix NeedsAlignedBaseOffset for composite R2R by davidwrighton · Pull Request #35231 · dotnet/runtime

davidwrighton · 2020-04-21T05:56:14Z

Synchronize and correct NeedsAlignedBaseOffset

Crossgen2 and the runtime had different models for when the base type size needed to be aligned up
This only caused issues for composite crossgen2 builds as the logic was heavily module focussed
Also, the IsInSameVersionBubble logic was non-functional for composite build

All of the following conditions must hold, or there shall be alignment inserted between a base and derived type

The derived type must declare that the base type is in the same version bubble as the derived type
The base type must declare that it was able to derive from its base type without needing alignment
The base type must not be instantiated over a valuetype where the valuetype instantiation contributes to the instance field layout of the base type, and that valuetype is not wholly defined in one bubble

Also add a test for various mixed combinations

davidwrighton · 2020-04-21T05:59:55Z

/azp list

azure-pipelines · 2020-04-21T06:00:00Z

janvorli · 2020-04-21T08:39:22Z

Exactly the same logic is now duplicated here and in the MethodTableBuilder::CheckLayoutDependsOnOtherModules. So it seems that this whole block can be replaced by

return pParentMT->GetClass()->HasLayoutDependsOnOtherModules();

trylek

LGTM, thanks for fixing these inconsistencies!

davidwrighton · 2020-04-24T00:42:28Z

Add a multitude of tests and fix issues related to composite, input bubble and regular r2r crossgen build. The tests will need some refactoring once @nattress finishes his test harness work, but for now there is a batch script.

- Crossgen2 and the runtime had different models for when the base type size needed to be aligned up - This only caused issues for composite crossgen2 builds as the logic was heavily module focussed - Also, the IsInSameVersionBubble logic was non-functional for composite build All of the following conditions must hold, or there shall be alignment inserted between a base and derived type - The derived type must declare that the base type is in the same version bubble as the derived type - The base type must declare that it was able to derive from its base type without needing alignment - The base type must not be instantiated over a valuetype where the valuetype instantiation contributes to the instance field layout of the base type, and that valuetype is not wholly defined in one bubble

crossgen, crossgen2, and crossgen2 with input bubble enable

Mostly works

davidwrighton · 2020-04-24T05:10:35Z

@dotnet/crossgen-contrib

@jkotas This changes the field layout for non-R2R to follow rules similar to R2R in some somewhat rare cases. I'd like your opinion on that.

It was a review of an earlier change

not the static layout.

jkotas · 2020-04-26T21:10:06Z

        m_VMFlags |= VMFLAG_LAYOUT_DEPENDS_ON_OTHER_MODULES;
    }
+
+    inline BOOL HasLayoutDependsOnOtherVersionBubbles()


Do we really need both HasLayoutDependsOnOtherModules and HasLayoutDependsOnOtherVersionBubbles ?

I would expect to have just the version bubble one.

jkotas · 2020-04-26T21:23:29Z

I am having hard time figuring out the reasons behind the rules. I would expect the rules to be:

All of the following conditions must hold, or there shall be alignment inserted between a base and derived type

The derived type must declare that the base type is in the same version bubble as the derived type
The base type must declare that its layout does not depend on types outside its version bubble

What am I missing?

davidwrighton · 2020-04-27T17:47:43Z

There are several issues fixed in this change, and the change description does not currently describe them well.

Many of the primitives that the calculation used returned subtly different values in the runtime and in crossgen2. As well as syncrhonizing the overall layout algorithm with the runtime. Fixing these issues is what fixed the composite build.
The second issue is what drove the push to a new algorithm and is actually not related to composite builds. The design of the existing algorithm relied on the concept that the various version bubbles were the same at both crossgen time and at runtime. Unfortunately, that assumption isn't true with input bubble style compilations in the style that we want to build for technologies such as layered containers.

Layer 1 - All assemblies AOT compiled here will be compiled with /inputbubble enabled.
System.Private.CoreLib.dll
System.*.dll

Layer 2
Asp.NET Dlls, or other content layered on top of the assemblies produced in layer 1. Also compiled with /inputbubble.

Layer 3
Application dlls, compiled with whatever policy the developer chooses (input bubble, normal R2R, no AOT, etc)

The concern is for the layout of a class Derived<SomeStruct>

Defined in Layer 1
class Base<T>
{
    public T _t;
}
class Derived<U> : Base<T>
{
    public U _u;
}

Defined in Layer 2
struct SomeStruct
{
    public byte _someByte;
}

For Derived<SomeStruct>, what is the offset of _u? An algorithm exclusively built on the version bubble rules would have difficulty in this situation.

The type clearly cannot exist during compilation of Layer1, as SomeStruct is within the set of assemblies defined in Layer1.

When Layer2 is being compiled, it will consider all components of Derived<SomeStruct> to be within the same version bubble, so it will generate code without extra alignment.

However, when running the application, the version bubble algorithm will declare that Base<SomeStruct> has cross version bubble dependent layout, and layout the type Derived<SomeStruct with extra alignment.

The new algorithm as written uses a stricter definition for version bubble when considering layout that is caused by a generic instantiation. In particular, the version bubble rule retreats to consider the indivisible unit of the module which will cause both runtime and crossgen time compilation of Derived<SomeStruct> to have the same layout.

The practical effect is that the /inputbubble switch becomes reliable as long as the assemblies statically depended on by the assembly under compilation are also compiled with the /inputbubble switch, or not compiled at all. The system no longer requires that ALL assemblies that might be compiled with the inputbubble switch be compiled knowing exactly the same set of references.

jkotas · 2020-04-27T19:51:44Z

Ok, your example makes sense.

Would it make sense to have the R2R layout to be based exclusively on module boundaries, and never consider version bubbles for it to keep the rules simple?

davidwrighton · 2020-04-27T20:34:45Z

I believe we would have to force ALL type layout to follow the module local rules, not just R2R layout. As you say, its much simpler, and less complex to explain, and probably less risky going forward. I'm not convinced this new algorithm worth the risk, as I don't have good data either way how much we win from the non-aligned layout optimization.

jkotas · 2020-04-27T20:55:09Z

I believe we would have to force ALL type layout to follow the module local rules, not just R2R layout.

What would be the example that demonstrates we need to force ALL type layout to follow the module local rules?

davidwrighton · 2020-04-27T23:18:40Z

Its the same example as above, except for some reason we were unable to use the R2R image for the assembly which defined Base<T> or Derived<T>. In that case, the code in layer 2 will assume a type layout for Derived<T> that isn't the same as the runtime will compute.

davidwrighton · 2020-04-27T23:51:49Z

@mangod9 I've confirmed that this fix addresses the failures seen in running asp.net with composite enabled.

jkotas · 2020-04-28T05:37:39Z

for some reason we were unable to use the R2R image for the assembly which defined Base or Derived

We may be unable to use code from the R2R image, but we should be still able to see that this is R2R image and it should follow the R2R layout rules.

davidwrighton · 2020-04-28T17:33:02Z

That's a detail I don't actually believe is certain, and I'm somewhat uncomfortable with relying on every dependent image to be R2R'd. It could happen, but we have historically had discussion about crossgening only some files, etc.

To be honest, I'm somewhat uncomfortable with the algorithm I wrote up as well, as it assumes that all crossgen used on a dependent must be compiled with inputbubble if some other module with inputbubble exists which depends on it. I feel this is somewhat more likely to be reliable, but again, its not something I'm convinced is a safe model.

trylek · 2020-04-28T19:38:25Z

I wonder whether part of the trouble with --inputbubble may be in that it was designed with this "all-or-nothing" policy in mind. Now we're trying to improve on composability, would it help to just ditch the switch and instead use a new type of reference to define "those modules that won't be compiled into the R2R executable currently being produced but logically form a part of its version bubble", something like -rb (reference-bubble)?

davidwrighton · 2020-04-28T19:59:16Z

@trylek you're right, but @jkotas talked after the meeting today and came up with an approach that doesn't require that complexity.

Require that all assemblies dependend on by an inputbubble assembly to be run through the crossgen2 compiler in some way. (To support the legitimate case where we may not wish to compile any methods in a binary, we will build a new switch for crossgen2 that will put an R2R header on an image, but not use it for anything.
Revert the algorithm to the model built for R2R in the past, except that the "module" that must be identical would allow for all of the contents of a single composite image to be considered as a single module.
Remove the existing version bubble handling in favor of the strict "module" concept above.
When we add module validation to the R2R product, if the image was compiled with inputbubble enabled, validate that the image is an R2R image.
If an image has R2R data, even if it isn't useable (due to an instruction set mismatch or something, or version number problem), the image shall be considered to be an R2R image for the purposes of this type layout algorithm.

…putbubble compiled assembly must be r2r compiled Fix R2RDump for composite images

mangod9 · 2020-04-29T05:18:30Z

+            while (true)
+            {
+                Thread.Sleep(1000);
+            s_testFailObj = o;


nit: indent

Ah, that was temporary logic in the test to make it easier to debug. I'll delete the new code.

mangod9 · 2020-04-29T05:21:51Z

  </data>
+  <data name="CompileNoMethodsOption" xml:space="preserve">
+    <value>True to not compile any methods into the R2R image (default = false)</value>
+  </data>


maybe: True to skip compiling methods into the R2R image?

Again, I should never be allowed to write customer visible text. :) I like your wording better.

jkotas

Looks much better :-)

trylek

Looks awesome, thanks David for driving this to successful resolution!

janvorli · 2020-04-29T16:26:06Z

+            }
+        }
+
+        // Indicate 


What does this comment mean?

I fell asleep before finishing writing it... update on the way along with use of an enum instead of an int.

janvorli · 2020-04-29T16:27:08Z

+        {
+            EcmaModule module = (EcmaModule)nonEcmaModule;
+            if (IsModuleInCompilationGroup(module))
+                return 2;


It would be nice to have enum or named constants for these 2 and 3.

Agreed. I'm putting an enum together which should make this code somewhat easier to parse.

janvorli · 2020-04-29T16:45:40Z

-            }
-        }
-#endif // FEATURE_READYTORUN_COMPILER
+        GetHalfBakedClass()->SetHasLayoutDependsOnOtherModules();


What is the case when this bit was not already set by the CheckLayoutDependsOnOtherModules?

Hmm, it appears to be legacy from the other algorithm I wrote up. I'll remove it, and verify that the tests all pass.

janvorli · 2020-04-29T16:53:57Z

+
+if "%COMPOSITENAME%"=="" goto done
+
+set BUILDCMD=%TESTBATCHROOT%\..\..\..\..\..\..\.dotnet\dotnet %CORE_ROOT%\crossgen2\crossgen2.dll -r %CORE_ROOT%\* -r %TESTINITIALBINPATH%\*.dll -o %TESTINITIALBINPATH%\%TESTTARGET_DIR%\%COMPOSITENAME%Composite.dll --composite %COMPILEARG%


I am surprised that "-r %CORE_ROOT%*" doesn't cause crossgen2 to fail for files that are not managed dlls.

Crossgen2 filters out native dlls automatically; the only reason we're using more explicit reference file groups in CLRTest.Crossgen.targets and build-test.cmd/sh is to make it a bit faster and get rid of the annoying warning messages.

janvorli · 2020-04-29T16:56:45Z

+
+if "%COMPOSITENAME%"=="" goto done
+
+set BUILDCMD=%TESTBATCHROOT%\..\..\..\..\..\..\.dotnet\dotnet %CORE_ROOT%\crossgen2\crossgen2.dll -r %CORE_ROOT%\* -r %TESTINITIALBINPATH%\*.dll -o %TESTINITIALBINPATH%\%TESTTARGET_DIR%\%COMPOSITENAME%Composite.dll --composite %COMPILEARG%


A nit - it would be nice to extract the dotnet path to a variable. I also think that it is not guaranteed that the .dotnet folder exists. I believe that Viktor told me some time ago that it is not created in case you have the same version of dotnet installed on your machine. I think the right way is to use the dotnet.cmd/sh script in the root of the repo that takes care of locating the dotnet exe.

This entire test harness of ugly batch files is temporary, to be replaced with whatever @nattress comes up with. The reason I use the exact path to the dotnet exe is that it allows for copying/pasting the printed build command and passing it as a launch argument to the debugger which saved me quite a lot of time during development. I don't intend to fix this before checkin.

janvorli · 2020-04-29T16:59:50Z

@@ -0,0 +1,322 @@
+echo off
+setlocal 
+set TESTDIR=%~dp0\..\..\..\..\..\..\artifacts\tests\coreclr\Windows_NT.x64.Debug\readytorun\crossboundarylayout\crossboundarytest\crossboundarytest


Should this script get the arch / build flavor as arguments?

In theory yes, but in practice, as this logic is entirely cross arch/os it only needs testing in one environment, and the goal is to delete these scripts as soon as @nattress produces a way to write these tests that is actually reasonable.

janvorli · 2020-04-29T17:03:40Z

@@ -0,0 +1,322 @@
+echo off


I wonder if this script (and the other ones) should be located in src/coreclr/tests/scripts instead.

janvorli

LGTM, just one tiny grammar nit.

…/ReadyToRunCompilationModuleGroupBase.cs Co-Authored-By: Jan Vorlicek <janvorli@microsoft.com>

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 21, 2020

davidwrighton requested review from janvorli and trylek April 21, 2020 05:56

janvorli reviewed Apr 21, 2020

View reviewed changes

trylek previously approved these changes Apr 21, 2020

View reviewed changes

davidwrighton marked this pull request as ready for review April 24, 2020 00:42

davidwrighton added 9 commits April 23, 2020 18:07

Interesting type layout test cases

984b2a8

Add tests for cases involving inheritance in a single module

ec46f20

Add testing for 45 different variations of compiling the tests with

9d247de

crossgen, crossgen2, and crossgen2 with input bubble enable

Use new layout algorithm

26ea12d

Mostly works

Make test crashes more diagnosable

c94a078

Passing Current Test Suite

a59000b

Remove halt from test

bdc07b4

Expand testing to cover more complex cases and in more thoroughness

39d28b2

davidwrighton force-pushed the fix_cg2_composite branch from 99cb4f7 to 39d28b2 Compare April 24, 2020 04:18

trylek reviewed Apr 24, 2020

View reviewed changes

Comment thread src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs

Field layout static-ness is entirely dependent on the instance layout,

c7f997a

not the static layout.

jkotas reviewed Apr 26, 2020

View reviewed changes

davidwrighton mentioned this pull request Apr 28, 2020

Validate modules that are part of a version bubble #31822

Closed

davidwrighton added 2 commits April 28, 2020 17:41

Rework of algorithm

2cd3f3b

Fix the tests to match the requirement that all dependencies of an in…

5a856dd

…putbubble compiled assembly must be r2r compiled Fix R2RDump for composite images

mangod9 reviewed Apr 29, 2020

View reviewed changes

Comment thread ...r/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs Outdated

mangod9 reviewed Apr 29, 2020

View reviewed changes

jkotas approved these changes Apr 29, 2020

View reviewed changes

trylek approved these changes Apr 29, 2020

View reviewed changes

janvorli reviewed Apr 29, 2020

View reviewed changes

Address code review feedback

c61651b

janvorli approved these changes Apr 29, 2020

View reviewed changes

Comment thread ...r/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs Outdated

mangod9 approved these changes Apr 29, 2020

View reviewed changes

Update src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler…

d1d0155

…/ReadyToRunCompilationModuleGroupBase.cs Co-Authored-By: Jan Vorlicek <janvorli@microsoft.com>

davidwrighton merged commit 68af9c3 into dotnet:master Apr 29, 2020

davidwrighton mentioned this pull request Apr 30, 2020

Crossgen2 composite / shared framework - assert(varDsc->lvDoNotEnregister || tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR)) #35077

Closed

jaredpar mentioned this pull request May 5, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

davidwrighton deleted the fix_cg2_composite branch April 20, 2021 17:42


		if "%COMPOSITENAME%"=="" goto done

		set BUILDCMD=%TESTBATCHROOT%\..\..\..\..\..\..\.dotnet\dotnet %CORE_ROOT%\crossgen2\crossgen2.dll -r %CORE_ROOT%\* -r %TESTINITIALBINPATH%\*.dll -o %TESTINITIALBINPATH%\%TESTTARGET_DIR%\%COMPOSITENAME%Composite.dll --composite %COMPILEARG%

Conversation

davidwrighton commented Apr 21, 2020

Uh oh!

davidwrighton commented Apr 21, 2020

Uh oh!

azure-pipelines Bot commented Apr 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Apr 24, 2020

Uh oh!

davidwrighton commented Apr 24, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Apr 26, 2020

Uh oh!

davidwrighton commented Apr 27, 2020

Uh oh!

jkotas commented Apr 27, 2020

Uh oh!

davidwrighton commented Apr 27, 2020

Uh oh!

jkotas commented Apr 27, 2020

Uh oh!

davidwrighton commented Apr 27, 2020

Uh oh!

davidwrighton commented Apr 27, 2020

Uh oh!

jkotas commented Apr 28, 2020

Uh oh!

davidwrighton commented Apr 28, 2020

Uh oh!

trylek commented Apr 28, 2020

Uh oh!

davidwrighton commented Apr 28, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!