Replace ReadFile/WriteFile usages with stdio #116203
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
| @@ -342,6 +343,18 @@ CreateFileWrapper( | |||
| return ret; | |||
| } | |||
|
|
|||
| int fopen_u16_wrapper(FILE** stream, const WCHAR* filename, const WCHAR* mode) | |||
There was a problem hiding this comment.
The long path normalization should be retained in long-term, because it's required for production code like assembly loading. Forward linking doesn't work for superpmi because utilcode is not linked to it. Moving all the normalization logic to coreclrminipal would require moving SString down. What's the recommended approach for this?
There was a problem hiding this comment.
Is the path normalization needed with CRT methods like fopen?
There was a problem hiding this comment.
It is needed for both fopen and fstream. I skipped fopen which is only used in development tools.
| @@ -138,7 +138,7 @@ unsigned int MethodContext::calculateRawFileSize() | |||
| 2 /* end canary '4', '2' */; | |||
| } | |||
|
|
|||
| unsigned int MethodContext::saveToFile(HANDLE hFile) | |||
There was a problem hiding this comment.
Is it safe to use STL in all superpmi code, including the shims/driver that executes in the same process?
If so, it can be refactored and simplified a lot.
There was a problem hiding this comment.
superpmi doesn't have the same servicing concerns as coreclr.dll, so I believe it's okay to use the STL throughout. @jkotas do you agree?
| @@ -632,26 +633,26 @@ HRESULT PEWriter::setDirectoryEntry(PEWriterSection *section, ULONG entry, ULONG | |||
| // across a dll boundary and use it. | |||
There was a problem hiding this comment.
Is this still a concern? How we have dynamically linked UCRT on Windows.
There was a problem hiding this comment.
IIRC Debug builds use static linked CRT
There was a problem hiding this comment.
Yes, debug builds use a static CRT when possible (C++/CLI tests are the only cases that don't).
There was a problem hiding this comment.
This specific comment is not a concern anymore. mscorpe.dll is not a separate library.
| LONG _cRef; | ||
| HANDLE _hFile; | ||
| LONG _cRef; | ||
| FILE* _fp; | ||
|
|
||
| }; |
There was a problem hiding this comment.
Can this file be deleted and FILE used instead?
There was a problem hiding this comment.
It implements the COM IStream interface, but not passed over in most usages. Event pipe usage can be replaced with conditional compilation on OS-native handle like AOT.
| @@ -5794,7 +5794,7 @@ void DumpHeaderDetails(IMAGE_COR20_HEADER *CORHeader, void* GUICookie) | |||
|
|
|||
| void WritePerfData(const char *KeyDesc, const char *KeyName, const char *UnitDesc, const char *UnitName, void* Value, BOOL IsInt) | |||
There was a problem hiding this comment.
I think this whole tracing can be deleted.
| #define ERROR_ALREADY_EXISTS 183L | ||
| #define ERROR_FILENAME_EXCED_RANGE 206L | ||
|
|
||
| HRESULT HRESULT_FROM_LAST_STDIO() |
There was a problem hiding this comment.
Is the errno to HRESULT conversion really needed? Where are the HRESULTs used?
There was a problem hiding this comment.
It's used in ilasm and pewriter, which is also used by ilasm. Ilasm uses the HRESULTs in IfFailGo.
Replaces the majority of ReadFile/WriteFile instances with stdio, and fstream for some superpmi cases.
Remaining instances are StgIO and superpmi/fileio, which uses the same file handles for memory map.
This PR aims to remove as much ReadFile/WriteFile instances with as little change. Opening earlier for some questions.