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.

Sign up for GitHub

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

Jump to bottom

Fix singlefile on OSX ARM64 #68845

Merged
merged 4 commits into from
May 4, 2022
Merged

Fix singlefile on OSX ARM64 #68845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d928ae
Same alignment in the bundle on OSX as on Linux
VSadov May 3, 2022
14aae0c
Extra VA gap between section in casse we run from sf bundle
VSadov May 3, 2022
1426d16
Rename prevSectionEnd -> prevSectionEndAligned and make it aligned.
VSadov May 3, 2022
b8e8cae
Suppress assert for now on OSX
VSadov May 3, 2022
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
21 changes: 11 additions & 10 deletions src/coreclr/pal/src/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2394,8 +2394,9 @@ void * MAPMapPEFile(HANDLE hFile, off_t offset)
goto doneReleaseMappingCriticalSection;
}

void* prevSectionEnd;
prevSectionEnd = (char*)loadedHeader + headerSize; // the first "section" for our purposes is the header
void* prevSectionEndAligned;
// the first "section" for our purposes is the header
prevSectionEndAligned = ALIGN_UP((char*)loadedHeader + headerSize, GetVirtualPageSize());
Copy link
Member Author

Choose a reason for hiding this comment

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

The does not change how we map. I just renamed prevSectionEnd and made it always aligned. I think it is more readable.


for (unsigned i = 0; i < numSections; ++i)
{
Expand All @@ -2412,7 +2413,7 @@ void * MAPMapPEFile(HANDLE hFile, off_t offset)
if ( (sectionBase < loadedHeader) // Did computing the section base overflow?
|| ((char*)sectionBase + currentHeader.SizeOfRawData < (char*)sectionBase) // Does the section overflow?
|| ((char*)sectionBase + currentHeader.SizeOfRawData > (char*)loadedHeader + virtualSize) // Does the section extend past the end of the image as the header stated?
|| (prevSectionEnd > sectionBase) // Does this section overlap the previous one?
|| (prevSectionEndAligned > sectionBase) // Does this section overlap the previous one?
)
{
ERROR_(LOADER)( "section %d is corrupt\n", i );
Expand All @@ -2435,12 +2436,12 @@ void * MAPMapPEFile(HANDLE hFile, off_t offset)
}

// Is there space between the previous section and this one? If so, add a PROT_NONE mapping to cover it.
if (prevSectionEnd < sectionBaseAligned)
if (prevSectionEndAligned < sectionBaseAligned)
{
palError = MAPRecordMapping(pFileObject,
loadedBase,
prevSectionEnd,
(char*)sectionBaseAligned - (char*)prevSectionEnd,
prevSectionEndAligned,
(char*)sectionBaseAligned - (char*)prevSectionEndAligned,
PROT_NONE);
if (NO_ERROR != palError)
{
Expand Down Expand Up @@ -2488,18 +2489,18 @@ void * MAPMapPEFile(HANDLE hFile, off_t offset)
}
#endif // _DEBUG

prevSectionEnd = ALIGN_UP((char*)sectionBase + currentHeader.SizeOfRawData, GetVirtualPageSize()); // round up to page boundary
prevSectionEndAligned = ALIGN_UP((char*)sectionBase + currentHeader.SizeOfRawData, GetVirtualPageSize()); // round up to page boundary
}

// Is there space after the last section and before the end of the mapped image? If so, add a PROT_NONE mapping to cover it.
char* imageEnd;
imageEnd = (char*)loadedBase + virtualSize; // actually, points just after the mapped end
if (prevSectionEnd < imageEnd)
if (prevSectionEndAligned < imageEnd)
{
palError = MAPRecordMapping(pFileObject,
loadedBase,
prevSectionEnd,
offset + (char*)imageEnd - (char*)prevSectionEnd,
prevSectionEndAligned,
offset + (char*)imageEnd - (char*)prevSectionEndAligned,
PROT_NONE);
if (NO_ERROR != palError)
{
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/R2RPEBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,12 +599,19 @@ protected override BlobBuilder SerializeSection(string name, SectionLocation loc

if (!_target.IsWindows)
{
const int RVAAlign = 1 << RVABitsToMatchFilePos;
if (outputSectionIndex > 0)
{
sectionStartRva = Math.Max(sectionStartRva, _sectionRVAs[outputSectionIndex - 1] + _sectionRawSizes[outputSectionIndex - 1]);

// when assembly is stored in a singlefile bundle, an additional skew is introduced
// as the streams inside the bundle are not necessarily page aligned as we do not
// know the actual page size on the target system.
// We may need one page gap of unused VA space before the next section starts.
// We will assume the page size is <= RVAAlign
sectionStartRva += RVAAlign;
Copy link
Member Author

@VSadov VSadov May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think I understand the layout mechanism more now. The way we construct sectionStartRva guarantees that sections will not overlap - that is if the file starts form offset 0.
That is not the case in a singlefile bundle. In the bundle everything has an additional file-wide offset when we map.

On ARM64 the file from a bundle will start at an offset that is 4K aligned. On systems with 4K page size (i.e. default Ubuntu) such aligned offset will be as good as starting form 0. Thus we do not see asserts on Linux.

On OSX ARM64 the page size is 16K, so we may need to "spill" a section past the end of the target VA region and that may cause VA regions to overlap, which trigger a failure in the loader and an assert.
Same could happen on unusually configured Linuxes with larger page sizes.

I think we can assume that 64K is enough to cover any reasonable page size and leaving 64K unused VA gap between sections will ensure that we will not overlap.

}

const int RVAAlign = 1 << RVABitsToMatchFilePos;
sectionStartRva = AlignmentHelper.AlignUp(sectionStartRva, RVAAlign);

int rvaAdjust = (location.PointerToRawData - sectionStartRva) & (RVAAlign - 1);
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/vm/peimagelayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ PEImageLayout* PEImageLayout::LoadConverted(PEImage* pOwner)
if (pFlat == NULL || !pFlat->CheckILOnlyFormat())
EEFileLoadException::Throw(pOwner->GetPathForErrorMessages(), COR_E_BADIMAGEFORMAT);

#ifdef TARGET_UNIX
// TODO: enable on OSX eventually
// right now we have binaries that will trigger this in a singlefile bundle.
#ifdef TARGET_LINUX
// we should not see R2R files here on Unix.
// ConvertedImageLayout may be able to handle them, but the fact that we were unable to
// load directly implies that MAPMapPEFile could not consume what crossgen produced.
Expand Down
10 changes: 5 additions & 5 deletions src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,17 @@ public TargetInfo(OSPlatform? os, Architecture? arch, Version targetFrameworkVer
throw new ArgumentException($"Invalid input: Unsupported Target Framework Version {targetFrameworkVersion}");
}

if (IsLinux && Arch == Architecture.Arm64)
if (IsWindows)
{
// We align assemblies in the bundle at 4K so that we can use mmap on Linux without changing the page alignment of ARM64 R2R code.
// We align assemblies in the bundle at 4K - per requirements of memory mapping API (MapViewOfFile3, et al).
// This is only necessary for R2R assemblies, but we do it for all assemblies for simplicity.
// See https://github.com/dotnet/runtime/issues/41832.
AssemblyAlignment = 4096;
}
else if (IsWindows)
else if (Arch == Architecture.Arm64)
{
// We align assemblies in the bundle at 4K - per requirements of memory mapping API (MapViewOfFile3, et al).
// We align assemblies in the bundle at 4K so that we can use mmap on Unix without changing the page alignment of ARM64 R2R code.
// This is only necessary for R2R assemblies, but we do it for all assemblies for simplicity.
// See https://github.com/dotnet/runtime/issues/41832.
Copy link
Member Author

@VSadov VSadov May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Once you get past the assert, you may still see crashes. I think they were because of this. We align files in the bundle on 4K for Linux ARM64, to make sure that ADRP assembly instructions are stable without performing relocations.

I think the same applies to OSX as well as it is specific to instruction set. The fix could be older than Apple Silicon and therefore covered Linux only.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that on osx-arm64 or unusually configured linux-arm64 systems, it is not 4K page aligned but something like 16K? In which case, can we use GetVirtualPageSize() instead of hardcoded 4K here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in a compile-time tool. It cold be a completely different platform/os.
We do not want arbitrary big alignment too, since it will increase the total size of the single file app.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also this is because of ADRP instruction, which operates with 4k offsets regardless of OS page.

AssemblyAlignment = 4096;
}
else
Expand Down