[crossgen2] Use Alignment to calculate class Offset

[t-mustafin]

ComputeAutoFieldLayout() function ignores calculated fieldSizeAndAlignment.Alignment and uses context.Target.LayoutPointerSize instead. It produces a problems if fieldSizeAndAlignment.Alignment > context.Target.LayoutPointerSize on arm for example.
I suggest to use maximum of that values to calculate offset.
Issue #38822

[t-mustafin]

cc @alpencolt @jkotas @mangod9 @davidwrighton

[t-mustafin]

cc @MichalStrehovsky

[MichalStrehovsky]

This method mostly tries to match

runtime/src/coreclr/src/vm/methodtablebuilder.cpp

Line 7894 in 6072e4d

VOID MethodTableBuilder::PlaceInstanceFields(MethodTable ** pByValueClassCache)

in the VM. It feels like what we actually miss are #if FEATURE_64BIT_ALIGNMENT such as this one:

runtime/src/coreclr/src/vm/methodtablebuilder.cpp

Lines 8145 to 8150 in 6072e4d

	#ifdef FEATURE_64BIT_ALIGNMENT
	if (GetHalfBakedClass()->IsAlign8Candidate()) {
	minAlign = 8;
	}
	else
	#endif // FEATURE_64BIT_ALIGNMENT

There's a couple other things under FEATURE_64BIT_ALIGNMENT that are not present in the crossgen2 code.

I'll let @davidwrighton take the "does this exactly match what the CoreCLR VM is doing?" parts since my knowledge of the CoreCLR type system is fairly limited.

[davidwrighton]

I've written a more comprehensive layout test (See https://github.com/davidwrighton/runtime/tree/layout_testing ) That test isn't ready to merge, as the amount of C# in the test is unreasonably large, but I believe it should be a good way to see that most of these cases are handled properly. I plan to run that test on arm32 tomorrow, or Monday, with this fix.

[davidwrighton]

I was unable to run the layout test to completion on the arm32 hardware I have available. I repeatedly saw crashes while attempting to run the JIT_GetRuntimeType helper. However, my analysis of the situation indicates that @MichalStrehovsky is correct, handling of the FEATURE_64BIT_ALIGNMENT behavior in crossgen2 is what needs to be matched. The particular fix here looks to be correct, and appears to match up with this snippet of the type loader in coreclr.

runtime/src/coreclr/src/vm/methodtablebuilder.cpp

Lines 8113 to 8119 in 6072e4d

	#ifdef FEATURE_64BIT_ALIGNMENT
	if (pByValueMT->RequiresAlign8())
	dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, 8);
	else
	#endif // FEATURE_64BIT_ALIGNMENT
	dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, TARGET_POINTER_SIZE);
	#endif // !(!defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4))

My analysis of the rest of the behavior of the runtime indicates that this is at least close to correct, but as I was unable to run the test suites, I could not confirm further details. If you fix the vairable use issue I mentioned here, I will approve this fix as it is strictly more correct than the previous state, but I do not have confidence that all issues have been found.

[davidwrighton]

This value doesn't represent the largetAlignment required, so please don't re-use the variable here. Define a new variable.

[t-mustafin]

Thanks, I defined new variable AlignmentRequired.

[davidwrighton]

As I said earlier, I'm not entirely convinced this fixes everything, but I think its a strict improvement over the current state.