Fix RVA field alignment #60
Conversation
|
Yes, confirmed on windows and linux. |
|
Please look at backporting the fix to 7. As per our discussion, ideally we would backport just the fix, not the rest of Cecil's changes. |
This ensures that section starts are aligned by adjusting the previous Range's length to ensure the computed start of a new Range meets the alignment requirements. It was done this way rather than just computing an aligned start for the new Range, because the TextMap assumes that the Ranges are contiguous - see for example GetNextRVA. GetNextRVA was used to compute the Code RVA, before adding that segment to the TextMap. This meant that the alignment (only added later) wasn't taken into account when writing out the code, but then later the TextMap was modified to include the alignment. This is fixed by first inserting an aligned zero-length Code segment before writing the code, then inserting it again once the length is known. Removing the Code alignment altogether would work too, but the alignment constants are kept in there, because it looks like they were added intentionally, even though the reason is unknown. Note that before this change, the start of the Code segment (at least on x64) was not 16-byte aligned in the first place, so this change will actually move the beginning of the Code segment.
| // Alignment of the code segment must be set before the code is written | ||
| // These alignment values are probably not necessary, but are being left in | ||
| // for now in case something requires them. | ||
| map.AddMap (TextSegment.Code, 0, !pe64 ? 4 : 16); |
There was a problem hiding this comment.
This does not look right to me. The AnyCPU binaries can run on 64-bit architectures and this is not going to guarantee sufficient alignment for them,
For example, consider static ReadOnlySpan<long> MySpan => new long[] { 1, 2, 3, 6, 7 };. The backing RVA field needs to have 8-byte alignment in this case.
There was a problem hiding this comment.
Ah ok, this extra alignment applies to IL code only (and it should not be needed it in the first place). The RVA alignment is handled elsewhere.
There was a problem hiding this comment.
We went back and forth on changing the code alignment, but eventually decided to keep the original logic.
Second attempt to fix the RVA field alignment. The fix is the same as #55, with an extra fix for the issue encountered in dotnet/runtime#79449 and the upstream jbevain#888. The tests are passing upstream now.
The problem was the use of
GetNextRVAto compute theCodeRVA, before adding that segment to the TextMap. This meant that the alignment (only added later) wasn't taken into account when writing out the code, but then later the TextMap was modified to include the alignment.I fixed this by first inserting an aligned zero-length
Codesegment before writing the code, then inserting it again once the length is known. Removing theCodealignment altogether would work too, but @vitek-karas and I were thinking it might be better to keep those alignment constants in there, because it looks like they were added intentionally, even though we don't know the reason.Note that before this change, the start of the
Codesegment (at least on x64) was not 16-byte aligned in the first place, so this change will actually move the beginning of theCodesegment.