Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update NativeAOT codegen and Crossgen2 for CreateSpan #63977

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/design/specs/Ecma-335-Augments.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ This is a list of additions and edits to be made in ECMA-335 specifications. It
- [Static Interface Methods](#static-interface-methods)
- [Covariant Return Types](#covariant-return-types)
- [Unsigned data conversion with overflow detection](#unsigned-data-conversion-with-overflow-detection)
- [Rules for IL rewriters](#rules-for-il-rewriters)

## Signatures

Expand Down Expand Up @@ -948,3 +949,12 @@ Conversions from floating-point numbers to integral values truncate the number t
on the top of the stack is reinterpreted as an unsigned value before the conversion.
Note that integer values of less than 4 bytes are extended to int32 (not native int) on the
evaluation stack.

## Rules for IL Rewriters

There are apis such as `System.Runtime.CompilerServices.RuntimeHelpers.CreateSpan<T>(...)` which require that the PE file have a particular structure. In particular, that api requires that the associated RVA of a FieldDef which is used to create a span must be naturally aligned over the data type that `CreateSpan` is instantiated over. There are 2 major concerns.

1. That the RVA be aligned when the PE file is constructed. This may be achieved by whatever means is most convenient for the compiler.
2. That in the presence of IL rewriters that the RVA remains aligned. This section descibes metadata which will be processed by IL rewriters in order to maintain the required alignment.

In order to maintain alignment, if the field needs alignment to be preserved, the field must be of a type locally defined within the module which has a Pack (§II.10.7) value of the desired alignment. Unlike other uses of the .pack directive, in this circumstance the .pack specifies a minimum alignment.
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 19, 2022

Choose a reason for hiding this comment

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

Should we enforce in the runtime that the field passed to CreateSpan is always of a locally defined type and the packing matches the alignment (unless ref emitted, I guess)?

That way we won't have IL Linker/crossgen2 breaking code that works in the absence of those tools. There's still a slight chance that the alignment in metadata is wrong, but it eliminates some of the risk. E.g. Roslyn sometimes avoids introducing the PrivateImplementationDetails type if it can use a primitive type and who knows if that codepath could kick in.

Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,9 @@ public virtual ISymbolNode GetFieldRvaData(FieldDesc field)
{
// Use the typical field definition in case this is an instantiated generic type
field = field.GetTypicalFieldDefinition();
int fieldTypePack = (field.FieldType as MetadataType)?.GetClassLayout().PackingSize ?? 1;
return NodeFactory.ReadOnlyDataBlob(NameMangler.GetMangledFieldName(field),
((EcmaField)field).GetFieldRvaData(), NodeFactory.Target.PointerSize);
((EcmaField)field).GetFieldRvaData(), Math.Max(NodeFactory.Target.PointerSize, fieldTypePack));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,17 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
}

ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly);
builder.RequireInitialPointerAlignment();
byte[] rvaData = GetRvaData(factory.Target.PointerSize, out int requiredAlignment);
builder.RequireInitialAlignment(requiredAlignment);
builder.AddSymbol(this);
builder.EmitBytes(GetRvaData(factory.Target.PointerSize));
builder.EmitBytes(rvaData);
return builder.ToObjectData();
}

private unsafe byte[] GetRvaData(int targetPointerSize)
private unsafe byte[] GetRvaData(int targetPointerSize, out int requiredAlignment)
{
int size = 0;
requiredAlignment = targetPointerSize;

MetadataReader metadataReader = _module.MetadataReader;
BlobReader metadataBlob = new BlobReader(_module.PEReader.GetMetadata().Pointer, _module.PEReader.GetMetadata().Length);
Expand All @@ -80,6 +82,7 @@ private unsafe byte[] GetRvaData(int targetPointerSize)
Debug.Assert(field.HasRva);

int currentSize = field.FieldType.GetElementSize().AsInt;
requiredAlignment = Math.Max(requiredAlignment, (field.FieldType as MetadataType)?.GetClassLayout().PackingSize ?? 1);
if (currentSize > size)
{
// We need to handle overlapping fields by reusing blobs based on the rva, and just update
Expand Down
2 changes: 1 addition & 1 deletion src/tests/JIT/Intrinsics/CreateSpan_il.il
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=16'
extends [System.Runtime]System.ValueType
{
.pack 1
.pack 4
.size 16
} // end of class '__StaticArrayInitTypeSize=16'

Expand Down