Cleanup most usages of Windows file mapping #121896
Conversation
github-actions
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
huoyaoyuan
added
area-PAL-coreclr
and removed
needs-area-label
| #ifdef HOST_WINDOWS | ||
| #define MEMORY_MAPPED_STRESSLOG_BASE_ADDRESS (void*)0x400000000000 | ||
| #else | ||
| #define MEMORY_MAPPED_STRESSLOG_BASE_ADDRESS nullptr |
There was a problem hiding this comment.
Is this removing support for memory mapped stresslog on non-Windows?
There was a problem hiding this comment.
Yes. NativeAOT copy of stresslog removes memory map support on all platforms. I haven't measured the impact.
src/coreclr/minipal/dn-memmap.h
Outdated
| #ifdef TARGET_WINDOWS | ||
| HANDLE m_hFileMapping; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
| #ifdef TARGET_WINDOWS | |
| HANDLE m_hFileMapping; | |
| #endif | |
This can be deleted from here (close the FileMapping handle at the end of the Open method instead)
| if (m_File) | ||
| delete m_File; |
There was a problem hiding this comment.
This can just be delete m_File;. Implementations of delete have handled nullptr for over 30 years.
There was a problem hiding this comment.
All of the existing code here are performing null test for delete. Should we prefer consistency here?
src/coreclr/ilasm/asmparse.h
Outdated
| if (m_File) | ||
| delete m_File; |
There was a problem hiding this comment.
| if (m_File) | |
| delete m_File; | |
| delete m_File; |
src/coreclr/ilasm/asmparse.h
Outdated
| 0, OPEN_EXISTING, 0, 0); | ||
| return (m_hFile == INVALID_HANDLE_VALUE) ? NULL : map_file(); | ||
| m_File = CreateMappedFile(moduleName); | ||
| if (m_File != nullptr && m_File->Size() > UINT_MAX) |
There was a problem hiding this comment.
Is m_File->Size() > UINT_MAX always needed? It isn't checked on all uses of CreateMappedFile()? Perhaps CreateMappedFile() should be updated for that check instead of using size_t.
There was a problem hiding this comment.
This would make it loose the ability to load files >4GB in ildasm. It should be none-issue since the PE header has limitation about size anyway.
src/coreclr/ildasm/ildasmpch.h
Outdated
| #include <limits.h> | ||
| #include <algorithm> | ||
| #include "dn-stdio.h" | ||
| #include "dn-memmap.h" |
There was a problem hiding this comment.
| #include "dn-memmap.h" | |
| #include <dn-memmap.h> |
src/coreclr/tools/metainfo/mdobj.cpp
Outdated
| if (f) | ||
| delete f; |
There was a problem hiding this comment.
| if (f) | |
| delete f; | |
| delete f; |
src/coreclr/tools/metainfo/mdobj.cpp
Outdated
|
|
||
| GetMapViewOfFile(szFile, &pbMapAddress, &dwFileSize); | ||
| MemoryMappedFile* f = CreateMappedFile(szFile); | ||
| if (f == nullptr || f->Size() >= UINT32_MAX) |
There was a problem hiding this comment.
We should use the same type, UINT32_MAX or UINT_MAX or MAXDWORD. Just be consistent.
| } | ||
| EX_CATCH | ||
| { | ||
| return nullptr; |
There was a problem hiding this comment.
| return nullptr; | |
| // Swallow all exceptions. |
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
3 participants
The remaining cases are superpmi (included in #121367) and PEImage, which involves LoadLibrary and we would want more future-proof abstraction.
All the cases included in this PR are transitively used by ilasm/ildasm. For stgio, I'm doing simple ifdef's on OS api, since it's too complicated around different I/O models, and we would get rid of it with new metadata library.