FieldRVA alignment #817

2021-12-10T15:00:41-06:00

davidwrighton commented

2021-12-10 15:00:41 -06:00

(Migrated from github.com)

In support of dotnet/runtime#60948 the linker (an assembly rewriter) will need to be able to preserve the alignment of RVA based fields which are to be used to create the data for CreateSpan<T> records

This is implemented by adding a concept that RVA fields detect their required alignment by examining the PackingSize of the type of the field (if the field type is defined locally in the module)

In support of dotnet/runtime#60948 the linker (an assembly rewriter) will need to be able to preserve the alignment of RVA based fields which are to be used to create the data for `CreateSpan<T>` records This is implemented by adding a concept that RVA fields detect their required alignment by examining the PackingSize of the type of the field (if the field type is defined locally in the module)

davidwrighton commented

2021-12-10 15:03:38 -06:00

(Migrated from github.com)

@jaredpar Does generating the Packing records for these types seem reasonable to you when implementing CreateSpan<T> If you approve of the general design, I'll go through the effort of submitting PRs to the various components across the ecosystem to make this work reliably.
@vitek-karas Could you do a code review of this? The intent is to build the underlying fix for dotnet/linker#2417

@jaredpar Does generating the Packing records for these types seem reasonable to you when implementing `CreateSpan<T>` If you approve of the general design, I'll go through the effort of submitting PRs to the various components across the ecosystem to make this work reliably. @vitek-karas Could you do a code review of this? The intent is to build the underlying fix for dotnet/linker#2417

vitek-karas (Migrated from github.com) reviewed

2021-12-10 15:31:33 -06:00

Mono.Cecil/AssemblyWriter.cs

					
				@ -1616,0 +1620,4 @@

							// as PackingSize is only used when examining a type local to the module being written.

							int align = 1;

							if (field.FieldType.IsDefinition && !field.FieldType.IsGenericInstance) {

I'm not sure that the IsDefinition is enough to check that it's in the same assembly. It will be true for object read from IL, but it might not be true for objects modified or created programatically. As far as I know it's perfectly valid to assign a TypeDefinition to the FieldType.

I'm not sure that the `IsDefinition` is enough to check that it's in the same assembly. It will be true for object read from IL, but it might not be true for objects modified or created programatically. As far as I know it's perfectly valid to assign a TypeDefinition to the FieldType.

davidwrighton commented

2022-01-03 12:02:12 -06:00

(Migrated from github.com)

@jaredpar, now that we're all back from holiday vacation, could you take a look and let me know if you agree with the idea of this fix. The issue raised by @vitek-karas needs to be fixed, but I'd like to know if you agree with the choice of encoding.

AaronRobinsonMSFT (Migrated from github.com) reviewed

2022-01-03 15:10:23 -06:00

Mono.Cecil.Metadata/Buffers.cs

					
				@ -266,0 +269,4 @@

				                        // Compute the number of bytes to align the current position.

				                        // Values of 0 will be written.

							WriteBytes (((position + align) & ~align) - position);

						}

Might be worthwhile to unpack this logic in a comment.

AaronRobinsonMSFT (Migrated from github.com) reviewed

2022-01-03 15:23:44 -06:00

Mono.Cecil.Metadata/Buffers.cs

					
				@ -266,0 +269,4 @@

				                        // Compute the number of bytes to align the current position.

				                        // Values of 0 will be written.

							WriteBytes (((position + align) & ~align) - position);

						}

                        // Compute the number of bytes to align the current position.
                        // Values of 0 will be written.
			WriteBytes (((position + align) & ~align) - position);

```suggestion // Compute the number of bytes to align the current position. // Values of 0 will be written. WriteBytes (((position + align) & ~align) - position); ```

AlekseyTs commented

2022-01-03 17:03:00 -06:00

(Migrated from github.com)

@davidwrighton

could you take a look and let me know if you agree with the idea of this fix. ... I'd like to know if you agree with the choice of encoding.

Could you please clearly state what is the ask for the compiler here?

@davidwrighton > could you take a look and let me know if you agree with the idea of this fix. ... I'd like to know if you agree with the choice of encoding. Could you please clearly state what is the ask for the compiler here?

davidwrighton commented

2022-01-03 19:23:54 -06:00

(Migrated from github.com)

@AlekseyTs The ask for the compiler here is that, now that the runtime implements dotnet/runtime#60948 when we integrate dotnet/roslyn#57123 into the mainline (non-hackathon) branch of the compiler, we will need to mark the FieldRVA that need to be aligned beyond a 1 byte alignment in such a way so that the various IL binary rewriting tools such as the IL Linker will not reduce/remove the alignment that the Roslyn compiler may decide is necessary.

Currently Roslyn doesn't have variable amounts of alignment, it always aligns data to a 4 byte alignment. (Technically Roslyn aligns to 8 byte offsets, but System.Reflection.Metadata only aligns the buffer passed to it by Roslyn at 4 bytes... I'm fixing that with a different change).

When Cecil based IL rewriting tools run over Roslyn generated code, they currently strip all alignment from FieldRVA's, resulting in less aligned data. This is acceptable, as all current uses of Field RVAs by Roslyn do not have alignment requirements.

However, when dotnet/runtime#60948 is used by Roslyn, field RVAs will now have real alignment requirements. This change is a proposal to provide a means for Roslyn to specify the needed alignment. The proposal is to use a currently valid set of bits (the PackingSize bits on locally defined structures) to indicate to Cecil based rewriting tools that they should apply the more stringent alignment requirement. While these bits have always been valid, historically for FieldRVA's they were always set to 0 or 1, which leaves an opportunity to utilize this encoding to solve this problem.

I want to get agreement from the Roslyn folks that this is a good direction to go for encoding this data, after which I will push to get this change integrated to the linker tool. And once the linker is in a good position, we will be able to implement the Roslyn feature.

@AlekseyTs The ask for the compiler here is that, now that the runtime implements dotnet/runtime#60948 when we integrate dotnet/roslyn#57123 into the mainline (non-hackathon) branch of the compiler, we will need to mark the FieldRVA that need to be aligned beyond a 1 byte alignment in such a way so that the various IL binary rewriting tools such as the IL Linker will not reduce/remove the alignment that the Roslyn compiler may decide is necessary. Currently Roslyn doesn't have variable amounts of alignment, it always aligns data to a 4 byte alignment. (Technically Roslyn aligns to 8 byte offsets, but System.Reflection.Metadata only aligns the buffer passed to it by Roslyn at 4 bytes... I'm fixing that with a different change). When Cecil based IL rewriting tools run over Roslyn generated code, they currently strip all alignment from FieldRVA's, resulting in less aligned data. This is acceptable, as all current uses of Field RVAs by Roslyn do not have alignment requirements. However, when dotnet/runtime#60948 is used by Roslyn, field RVAs will now have real alignment requirements. This change is a proposal to provide a means for Roslyn to specify the needed alignment. The proposal is to use a currently valid set of bits (the PackingSize bits on locally defined structures) to indicate to Cecil based rewriting tools that they should apply the more stringent alignment requirement. While these bits have always been valid, historically for FieldRVA's they were always set to 0 or 1, which leaves an opportunity to utilize this encoding to solve this problem. I want to get agreement from the Roslyn folks that this is a good direction to go for encoding this data, after which I will push to get this change integrated to the linker tool. And once the linker is in a good position, we will be able to implement the Roslyn feature.

AlekseyTs commented

2022-01-04 09:54:28 -06:00

(Migrated from github.com)

The proposal is to use a currently valid set of bits (the PackingSize bits on locally defined structures)

Sorry, it is still not clear to me what exactly compiler is expected to do. Is it supposed to specify ClassLayout.PackingSize for all user-defined structures? What is the value that compiler is supposed to use? BTW, I think compiler doesn't specify this information for user-defined types, unless StructLayoutAttribute is explicitly applied in source. Also, an example of current IL and desired IL for a small specific scenario might help.

> The proposal is to use a currently valid set of bits (the PackingSize bits on locally defined structures) Sorry, it is still not clear to me what exactly compiler is expected to do. Is it supposed to specify _ClassLayout.PackingSize_ for all user-defined structures? What is the value that compiler is supposed to use? BTW, I think compiler doesn't specify this information for user-defined types, unless _StructLayoutAttribute_ is explicitly applied in source. Also, an example of current IL and desired IL for a small specific scenario might help.

davidwrighton commented

2022-01-04 13:28:43 -06:00

(Migrated from github.com)

@AlekseyTs Ah, no. The idea is that when there is a chunk of raw data embedded into a PE file we generate a static field with an RVA to it. That static field exists within a <PrivateImplementationDetails> type, and Roslyn will autogenerate another type which defines the size of the static data. Currently the convention is to name them something like __StaticArrayInitTypeSize=15552. At the current moment, those types are always created with a PackingSize of 1. The idea would be to add a new concept which is when the field RVA needs to be aligned beyond 1 byte of alignment, that the Roslyn compiler will create StaticArrayInit type with a higher packing size, matching the alignment requirement of the RVA.

The canonical example is something like

ReadOnlySpan<int> myIntData = new int[]{1,2,3,4,5};

Today that turns into an allocation of an array via the Array.Initialize method....
The IL is something like...

ldc.i4 5
newarr int
dup
ldtoken valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=20' '0107A6D71D367FA901A73B2343D07B198CBB249E11715E27F2A077A666D82476'
call       void [System.Runtime]System.Runtime.CompilerServices.RuntimeHelpers::InitializeArray(class [System.Runtime]System.Array, valuetype [System.Runtime]System.RuntimeFieldHandle)
call       valuetype [System.Runtime]System.ReadOnlySpan`1<!0> valuetype [System.Runtime]System.ReadOnlySpan`1<int32>::op_Implicit(!0[])
stloc.0

When the feature above is done we will instead do something like...

ldtoken valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=20Align=4' '0107A6D71D367FA901A73B2343D07B198CBB249E11715E27F2A077A666D82476'
call       valuetype [System.Runtime]System.ReadOnlySpan`1<!!0> [System.Runtime]System.Runtime.CompilerServices.RuntimeHelpers::CreateSpan<int>(valuetype [System.Runtime]System.RuntimeFieldHandle)
stloc.0

The critical detail here is that the Roslyn generated StaticArrayInitType will need to be somewhat different, so that IL rewriters can maintain the needed alignment.

@AlekseyTs Ah, no. The idea is that when there is a chunk of raw data embedded into a PE file we generate a static field with an RVA to it. That static field exists within a `<PrivateImplementationDetails>` type, and Roslyn will autogenerate another type which defines the size of the static data. Currently the convention is to name them something like `__StaticArrayInitTypeSize=15552`. At the current moment, those types are always created with a PackingSize of 1. The idea would be to add a new concept which is when the field RVA needs to be aligned beyond 1 byte of alignment, that the Roslyn compiler will create StaticArrayInit type with a higher packing size, matching the alignment requirement of the RVA. The canonical example is something like ```C# ReadOnlySpan<int> myIntData = new int[]{1,2,3,4,5}; ``` Today that turns into an allocation of an array via the Array.Initialize method.... The IL is something like... ``` ldc.i4 5 newarr int dup ldtoken valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=20' '0107A6D71D367FA901A73B2343D07B198CBB249E11715E27F2A077A666D82476' call void [System.Runtime]System.Runtime.CompilerServices.RuntimeHelpers::InitializeArray(class [System.Runtime]System.Array, valuetype [System.Runtime]System.RuntimeFieldHandle) call valuetype [System.Runtime]System.ReadOnlySpan`1<!0> valuetype [System.Runtime]System.ReadOnlySpan`1<int32>::op_Implicit(!0[]) stloc.0 ``` When the feature above is done we will instead do something like... ``` ldtoken valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=20Align=4' '0107A6D71D367FA901A73B2343D07B198CBB249E11715E27F2A077A666D82476' call valuetype [System.Runtime]System.ReadOnlySpan`1<!!0> [System.Runtime]System.Runtime.CompilerServices.RuntimeHelpers::CreateSpan<int>(valuetype [System.Runtime]System.RuntimeFieldHandle) stloc.0 ``` The critical detail here is that the Roslyn generated StaticArrayInitType will need to be somewhat different, so that IL rewriters can maintain the needed alignment.

AlekseyTs commented

2022-01-04 15:15:48 -06:00

(Migrated from github.com)

Thank you, @davidwrighton. This clarified things a lot, How is compiler supposed to determine the value to use as the PackingSize?

Thank you, @davidwrighton. This clarified things a lot, How is compiler supposed to determine the value to use as the ```PackingSize```?

davidwrighton commented

2022-01-04 15:47:35 -06:00

(Migrated from github.com)

The CreateSpan<T> api has various requirements. In particular, the T can only be primitive types, and the alignment requirement for the data is the same as the size of the primitive type. So, a byte would need 1 byte alignment, an int would need 4 byte alignment, and a double would need 8 byte alignment. This proposal is a bit more general purpose than that, allowing arbitrary packing sizes, but I propose that only as Cecil is used for arbitrary IL rewriting, I want to make this update only once, and if we have a new type that we allow such as Vector128<T> or something, having a mechanism that can align at arbitrary higher alignments will allow us to avoid updating even more of the ecosystem.

The `CreateSpan<T>` api has various requirements. In particular, the T can only be primitive types, and the alignment requirement for the data is the same as the size of the primitive type. So, a `byte` would need 1 byte alignment, an `int` would need 4 byte alignment, and a `double` would need 8 byte alignment. This proposal is a bit more general purpose than that, allowing arbitrary packing sizes, but I propose that only as Cecil is used for arbitrary IL rewriting, I want to make this update only once, and if we have a new type that we allow such as `Vector128<T>` or something, having a mechanism that can align at arbitrary higher alignments will allow us to avoid updating even more of the ecosystem.

AlekseyTs commented

2022-01-04 15:56:35 -06:00

(Migrated from github.com)

If the size of the underlying value (the T) is going to clearly determine the value to specify as the PackingSize, I think there should be no problem to implement this in compiler.

If the size of the underlying value (the `T`) is going to clearly determine the value to specify as the `PackingSize`, I think there should be no problem to implement this in compiler.

davidwrighton commented

2022-01-04 16:07:03 -06:00

(Migrated from github.com)

Thank you. I will take it that you think this is a good way for the compiler to encode this detail and move on to addressing the comments from @AaronRobinsonMSFT and @vitek-karas.

jaredpar commented

2022-01-06 12:20:14 -06:00

(Migrated from github.com)

@davidwrighton do we have an uber issue tracking all of the remaining work to complete this feature? Wanted to track roslyn work and link it in there.

davidwrighton commented

2022-01-11 20:01:47 -06:00

(Migrated from github.com)