CleanUp GetFileAttribute(Ex) in PAL #116096

huoyaoyuan · 2025-05-29T14:32:10Z

First step of moving file IO to minipal.

huoyaoyuan · 2025-05-29T14:37:01Z

src/native/minipal/file.c

+// The normalization examples are :
+//  C:\foo\<long>\bar   => \\?\C:\foo\<long>\bar
+//  \\server\<long>\bar => \\?\UNC\server\<long>\bar
+static WCHAR* NormalizePath(const WCHAR* path)


This is yet-another duplicate of the path normalization algorithm:

corehost\hostmisc\longfile.native.cpp, implemented with std::wstring

utilcode\longfilepathwrappers.cpp, implemented with SString

PathInternal.EnsureExtendedPrefixIfNeeded, implemented with managed string

When we eventually move all Win32 API to minipal, the utilcode version should be replaced by minipal version. Implementing this in pure C is really error-prone and unpleasable.

huoyaoyuan · 2025-05-29T14:38:42Z

src/native/minipal/file.c

+    if (ret)
+    {
+        attributes->size = stat_data.st_size;
+        attributes->lastWriteTime = UnixTimeToWin32FileTime(stat_data.st_mtim);


minipal/time is already requiring timespec, let's see if all platforms are having timespec available in stat.

huoyaoyuan · 2025-05-29T14:40:49Z

src/native/minipal/file.c

+        attributes->size = stat_data.st_size;
+        attributes->lastWriteTime = UnixTimeToWin32FileTime(stat_data.st_mtim);


This somehow overlaps with libs/pal_io.c, the differences are:

The UTF16/UTF8 transformation is done at managed side for libs, but minipal needs cross-platform representation which prefers UTF16

This only includes the portion needed for native runtime, in libs there are much more information exposed.

Thus I think it's better to keep them separated.

huoyaoyuan · 2025-05-29T14:42:16Z

src/native/minipal/file.h

+    uint64_t lastWriteTime; // Windows FILETIME precision
+} minipal_file_attr_t;
+
+bool minipal_file_get_attributes_utf16(const char16_t* path, minipal_file_attr_t* attributes);


pal/mstypes.h defines WCHAR as char16_t, which is incompatible with unsigned short from minipal/types.h. Since pal uses char16_t unconditionally, I assume it's broad available.

which is incompatible with unsigned short

What was the error when you used CHAR16_T? You may need to sprinkle a few (CHAR16_T*)var at call sites like we did for utf conversion API call sites.

Just char16_t* and unsigned short* is incompatible. Yes there are casts like

runtime/src/coreclr/minipal/Unix/dn-u16.cpp

Line 12 in 1aa4568

return minipal_u16_strlen((CHAR16_T*)str);

On Windows, it is defined as wchar_t, while on Unix it is defined as unsigned short because char16_t is not available everywhere (as seen in current CI errors). I checked various platform headers when defining CHAR16_T and unsigned short was close enough (they use some flexible type but this assumption just works across our supported platforms).

jkotas · 2025-05-29T14:49:50Z

src/coreclr/md/compiler/mdutil.cpp


        // The cache is locked for read, so the list will not change.

        // Figure out the size and timestamp of this file


What is this cache used for? Do we still need it? Would it be better to delete it?

It's called by IMetaDataDispenser::OpenScope and reusing the same object for same file. I don't get a good insight for its impact and performance of creating new RegMeta instance.

I am deleting it in #116126. Once that change goes through, we should not need GetFileAttributes in the minipal.

jkotas · 2025-05-29T14:55:27Z

I/O for core runtime should fall into two categories:

Generic I/O, like open a file for logging. We should use standard C runtime for that.
Specialized I/O, like mapping executables into memory. There should be a very few of these functions and they are likely to fit better into CoreCLR-specific PAL than a generic minipal.

Things that do not fall into any of these two categories - like the metadata cache you have changed - look suspect. They should be scrutinized heavily.

huoyaoyuan · 2025-05-29T15:06:37Z

I/O for core runtime should fall into two categories:

Generic I/O, like open a file for logging. We should use standard C runtime for that.

Specialized I/O, like mapping executables into memory. There should be a very few of these functions and they are likely to fit better into CoreCLR-specific PAL than a generic minipal.

I did some attempt for that. For CRT I/O, the biggest problem is the string encoding of fopen. On Windows there is _wfopen available, but it would need some wrapping to handle the encoding difference. Should these functions taking file names go to shared minipal? Should they handle long path?
Using fstream is much more easier, but it's limited to non in-process components.
mmap is actually not that rare and even used by ilasm for input. Putting them under coreclr minipal looks better.
For out-of-process develop component like superpmi, it should be easier to switch to fstream instead.

jkotas · 2025-05-29T15:40:11Z

fopen

JIT has fopen_utf8 wrapper to solve similar problem. We can have u16 variant of it.

AaronRobinsonMSFT · 2025-05-29T17:01:33Z

src/coreclr/pal/tests/palsuite/file_io/WriteFile/test1/WriteFile.cpp


 	//To delete file need to make it normal
-	if(!SetFileAttributesA(szReadOnlyFile,FILE_ATTRIBUTE_NORMAL))
+	if((last_error = chmod(szReadOnlyFile, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)))


Should this also have the != 0 like above?

src/native/minipal/file.c

This reverts commit 9cc6456.

jkotas · 2025-05-31T03:10:46Z

src/coreclr/pal/inc/pal.h

-#define GetFileAttributes GetFileAttributesA
-#endif
-
 typedef enum _GET_FILEEX_INFO_LEVELS {


Delete these as well enums/types as well

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

jkotas · 2025-05-31T13:47:28Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2025-05-31T13:47:45Z

Azure Pipelines successfully started running 1 pipeline(s).

huoyaoyuan · 2025-05-31T15:05:48Z

error: use of undeclared identifier '_MAX_PATH'

It's a missed case of #115986. Is the automated trigger for PAL pipeline enabled now?

am11 · 2025-05-31T15:17:58Z

Missing #116096 (comment).

jkotas · 2025-05-31T19:16:09Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2025-05-31T19:16:26Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2025-05-31T19:41:51Z

FAILED: file_io/errorpathnotfound/test2/paltest_errorpathnotfound_test2. Exit code: 1

Thread waited for 2138803459 ms! (Acceptable delta: 300)
FAILED: threading/WaitForSingleObject/WFSOExMutexTest/paltest_waitforsingleobject_wfsoexmutextest. Exit code: 1

huoyaoyuan · 2025-06-01T17:41:13Z

FAILED: file_io/errorpathnotfound/test2/paltest_errorpathnotfound_test2. Exit code: 1

This is removed but missed from test list.

FAILED: threading/WaitForSingleObject/WFSOExMutexTest/paltest_waitforsingleobject_wfsoexmutextest. Exit code: 1

This doesn't look related.

jkotas · 2025-06-01T19:09:29Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2025-06-01T19:09:47Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas

Thanks

huoyaoyuan added 8 commits May 29, 2025 21:20

Basic file and long path normalization

dd12321

Compiling version of GetFileAttributesEx

31451c6

Use stat in superpmi

350329f

Cleanup GetFileAttributesA/W

8877ec1

Cleanup usages of GetFileAttributesExW

ab71aae

Cleanup GetFileAttributesExW

7695b5e

Cleanup SetFikeAttributesA/W

77f4e9a

Update configuration

40e466d

github-actions bot added the area-PAL-coreclr label May 29, 2025

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

huoyaoyuan commented May 29, 2025

View reviewed changes

Don't include uchar.h on apple

9cc6456

jkotas reviewed May 29, 2025

View reviewed changes

huoyaoyuan marked this pull request as draft May 29, 2025 15:08

AaronRobinsonMSFT reviewed May 29, 2025

View reviewed changes

build-analysis bot mentioned this pull request May 29, 2025

System.OperationCanceledException : The operation was canceled. dotnet/dnceng#5278

Closed

3 tasks

huoyaoyuan added 4 commits May 30, 2025 10:29

Revert "Don't include uchar.h on apple"

d01029d

This reverts commit 9cc6456.

Remove minipal/file

713dc46

Merge branch 'main'

0b980aa

Cleanup leftover

5a2455e

huoyaoyuan changed the title ~~CleanUp GetFileAttribute(Ex) and move to minipal~~ CleanUp GetFileAttribute(Ex) in PAL May 31, 2025

huoyaoyuan marked this pull request as ready for review May 31, 2025 01:40

jkotas reviewed May 31, 2025

View reviewed changes

src/coreclr/tools/superpmi/superpmi-shared/methodcontextreader.cpp Show resolved Hide resolved

Cleanup more

8922871

Fix pal test build

ae82587

Use != 0 consistently

1a3847c

This was referenced May 31, 2025

[wasm][debugger] Debugger tests disabled #89409

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

build-analysis bot mentioned this pull request May 31, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Cleanup removed test

e920c2b

jkotas mentioned this pull request Jun 1, 2025

Test failure: JIT/Regression/JitBlue/DevDiv_255294/DevDiv_255294/DevDiv_255294.cmd #106675

Closed

jkotas approved these changes Jun 1, 2025

View reviewed changes

jkotas merged commit a87245f into dotnet:main Jun 1, 2025
137 of 148 checks passed

huoyaoyuan deleted the minipal/file branch June 2, 2025 02:15

github-actions bot locked and limited conversation to collaborators Jul 2, 2025

		attributes->size = stat_data.st_size;
		attributes->lastWriteTime = UnixTimeToWin32FileTime(stat_data.st_mtim);


		// The cache is locked for read, so the list will not change.

		// Figure out the size and timestamp of this file

CleanUp GetFileAttribute(Ex) in PAL #116096

CleanUp GetFileAttribute(Ex) in PAL #116096

Uh oh!

Conversation

huoyaoyuan commented May 29, 2025

Uh oh!

huoyaoyuan May 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 29, 2025

Uh oh!

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

Uh oh!

Uh oh!

jkotas commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas commented May 31, 2025

Uh oh!

azure-pipelines bot commented May 31, 2025

Uh oh!

huoyaoyuan commented May 31, 2025

Uh oh!

am11 commented May 31, 2025

Uh oh!

jkotas commented May 31, 2025

Uh oh!

azure-pipelines bot commented May 31, 2025

Uh oh!

jkotas commented May 31, 2025

Uh oh!

huoyaoyuan commented Jun 1, 2025

Uh oh!

jkotas commented Jun 1, 2025

Uh oh!

azure-pipelines bot commented Jun 1, 2025

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huoyaoyuan May 29, 2025 •

edited

Loading

huoyaoyuan commented May 29, 2025 •

edited

Loading