Replace ReadFile/WriteFile usages with stdio #116203

huoyaoyuan · 2025-06-02T07:27:40Z

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.

huoyaoyuan · 2025-06-02T07:32:41Z

src/coreclr/utilcode/longfilepathwrappers.cpp

@@ -342,6 +343,18 @@ CreateFileWrapper(
    return ret;
 }

+int fopen_u16_wrapper(FILE** stream, const WCHAR* filename, const WCHAR* mode)


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?

Is the path normalization needed with CRT methods like fopen?

It is needed for both fopen and fstream. I skipped fopen which is only used in development tools.

huoyaoyuan · 2025-06-02T07:35:28Z

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

@@ -138,7 +138,7 @@ unsigned int MethodContext::calculateRawFileSize()
           2 /* end canary '4', '2' */;
 }

-unsigned int MethodContext::saveToFile(HANDLE hFile)


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.

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?

I think it's better to seperate superpmi out, to ease review and avoid unnecessary conversion of CreateFile->fopen->fstream.

huoyaoyuan · 2025-06-03T14:02:21Z

src/coreclr/dlls/mscorpe/pewriter.cpp

@@ -632,26 +633,26 @@ HRESULT PEWriter::setDirectoryEntry(PEWriterSection *section, ULONG entry, ULONG
 // across a dll boundary and use it.


Is this still a concern? How we have dynamically linked UCRT on Windows.

IIRC Debug builds use static linked CRT

Yes, debug builds use a static CRT when possible (C++/CLI tests are the only cases that don't).

This specific comment is not a concern anymore. mscorpe.dll is not a separate library.

jkotas · 2025-06-04T23:31:38Z

src/coreclr/inc/fstream.h

-        LONG                                _cRef;
-        HANDLE                              _hFile;
+        LONG   _cRef;
+        FILE*  _fp;

 };


Can this file be deleted and FILE used instead?

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.

jkotas · 2025-06-04T23:36:00Z

src/coreclr/ildasm/dasm.cpp

@@ -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)


I think this whole tracing can be deleted.

jkotas · 2025-06-04T23:39:47Z

src/coreclr/minipal/Unix/dn-stdio.cpp

+#define ERROR_ALREADY_EXISTS 183L
+#define ERROR_FILENAME_EXCED_RANGE 206L
+
+HRESULT HRESULT_FROM_LAST_STDIO()


Is the errno to HRESULT conversion really needed? Where are the HRESULTs used?

It's used in ilasm and pewriter, which is also used by ilasm. Ilasm uses the HRESULTs in IfFailGo.

huoyaoyuan · 2025-06-06T09:57:16Z

src/coreclr/minipal/Windows/dn-stdio.cpp

+
+int fopen_u16(FILE** stream, const WCHAR* path, const WCHAR* mode)
+{
+    return _wfopen_s(stream, path, mode);


_wfopen is warning by msvc, but fopen is not. Is it safely allowed by SDL? Should the shape of wrapper API be fopen instead of fopen_s?

huoyaoyuan · 2025-06-08T03:21:43Z

Tests are passing now. This is ready for review.

mangod9 · 2025-06-30T14:02:49Z

There is a merge conflict. Assume this is still ready to review?

jkotas · 2025-07-01T05:13:42Z

src/coreclr/minipal/Windows/dn-stdio.cpp

+
+HRESULT HRESULT_FROM_LAST_STDIO()
+{
+    return HRESULT_FROM_WIN32(::GetLastError());


Is GetLastError correct here? C runtime methods are documented to return error in errno.

I can't find any better mapping from errno, the original approach was using GetLastError when calling Windows API directly. CRT functions should also be preserving the LastError when calling Windows API. It's not used in important paths (only ilasm, ildasm and multicorejit).

CRT functions should also be preserving the LastError when calling Windows API

They happen to not stomp on the last error most of the time. I do not think they are preserving it intentionally 100%.

The errno mapping and the long path support parts of this PR are not pretty. I have not figured out what to do about it yet...

jkotas · 2025-07-01T05:16:03Z

src/coreclr/minipal/Unix/dn-stdio.cpp

+
+    FILE* result = fopen(pathU8, modeU8);
+
+    delete[] pathU8;


Can these delete[]s overwrite errno?

Maybe, if the implementation calls free etc.

jkotas · 2025-07-01T05:19:11Z

src/coreclr/minipal/Windows/dn-stdio.cpp

+
+int fsetpos_64(FILE* stream, int64_t pos)
+{
+    fpos_t fpos = (fpos_t)pos;


Why do we need fpos_t cast here? Is there a risk of 64-bit -> 32-bit truncation?

fpos_t is always defined as long long on Windows so there's no risk. I don't remember why it was here.

huoyaoyuan added 16 commits June 2, 2025 11:02

fopen_u16

bc0aeaa

Return errno

f21899c

Add fgetsize

91a65d8

General ReadFile

da5e47d

General WriteFile

8d87a78

Better error code

fbe663c

CFileStream

b8a70b5

Use stdio in superpmi

727b55b

superpmi batch 2

67943e6

Use fstream for string/number extracting

dee1622

Superpmi batch 3

3f9913d

Fix missing fflush

605b939

Use fprintf instead

970a294

Use 64bit seek/tell

8192513

Fix build

de36695

Wrap long path

c481f02

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 2, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 2, 2025

huoyaoyuan added area-PAL-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 2, 2025

huoyaoyuan commented Jun 2, 2025

View reviewed changes

Fix gcc build

d44f02f

huoyaoyuan commented Jun 3, 2025

View reviewed changes

jkotas reviewed Jun 4, 2025

View reviewed changes

huoyaoyuan added 4 commits June 5, 2025 10:12

Merge branch 'main' into pal/stdio

926a46e

Fix msvc build

b7f56c1

Cleanup CFileStream usage

e929448

Port aot implementation of eventpipe file operations

492473d

huoyaoyuan added 5 commits June 5, 2025 13:40

Cleanup fstream

6218277

Delete outdated comment in pewriter

7a7950a

Remove PerfData in ildasm

c1aab56

Revert changes in superpmi

3cfb4b3

Fix build

4894d4c

huoyaoyuan force-pushed the pal/stdio branch from 0e18247 to 4894d4c Compare June 5, 2025 16:11

Fix missing fclose

791551d

huoyaoyuan commented Jun 6, 2025

View reviewed changes

Fix string terminator

e0071ef

huoyaoyuan force-pushed the pal/stdio branch from 58f2359 to e0071ef Compare June 7, 2025 09:53

huoyaoyuan marked this pull request as ready for review June 7, 2025 11:42

jkotas reviewed Jul 1, 2025

View reviewed changes

huoyaoyuan added 3 commits July 2, 2025 14:11

Merge branch 'main'

02b95d7

Remove unnecessary cast

8ad7cf7

Preserve errno before delete[]

78f8ffc

build-analysis bot mentioned this pull request Jul 2, 2025

browser-wasm Windows build error #116746

Open

		@@ -632,26 +633,26 @@ HRESULT PEWriter::setDirectoryEntry(PEWriterSection *section, ULONG entry, ULONG
		// across a dll boundary and use it.

		@@ -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)

Replace ReadFile/WriteFile usages with stdio #116203

Are you sure you want to change the base?

Replace ReadFile/WriteFile usages with stdio #116203

Conversation

huoyaoyuan commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Jun 8, 2025

Uh oh!

mangod9 commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huoyaoyuan Jun 5, 2025 •

edited

Loading

jkotas Jun 4, 2025 •

edited

Loading