Skip to content

Conversation

mpfaff
Copy link
Contributor

@mpfaff mpfaff commented Apr 26, 2025

Release notes are here


This was suggested here and is another step towards fixing #23643.

These changes decrease the stack usage of realpathW by 64 KB.

I took the liberty of additionally documenting that realpathW will never access the pathname after it starts writing to out_buffer and took advantage of that to avoid moving the WTF-16 buffer into many call-sites. If this is undesirable, I can remove those particular changes.


As an aside, I noticed that QueryObjectName silently aligns up its buffer to @alignOf(OBJECT_NAME_INFORMATION) (which has the same alignment as pointers). Should this be documented on the function and in functions that call it so that callers know that using a std.os.windows.PATH_MAX_WIDE length buffer could technically fail with error.NameTooLong?

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 19aacf3 to 74a493e Compare April 26, 2025 01:56
@squeek502
Copy link
Member

To follow the standard deprecation procedure:

  • realpathW should remain for one release cycle, but with a doc comment noting that it is deprecated
  • Add a realpathW2 function with the new implementation and use that
  • After one release cycle, realpathW2 will be renamed to realpathW

Here's a previous example: #19844

@mpfaff
Copy link
Contributor Author

mpfaff commented Apr 26, 2025

I see that readLinkW has the same behaviour, converting the output to WTF-8. Would you like me to cover that in this PR as well, or leave it for another?

Regardless, here are some preliminary notes about changes to that function:

I think we would want to modify std.os.windows.ntToWin32Namespace by replacing the PathSpace return type with an out_buffer: []u16 parameter and []u16 return type. This way we would be able to modify the functions all the way up the chain to readLinkW to use out_buffer: []u16 buffer without needing that intermediate PathSpace on the stack. If I'm understanding the ntToWin32Namespace function correctly, we could easily return error.NameTooLong if out_buffer is not long enough (path.len - 3 or path.len - 6 depending on branching).

All in all, I believe that would require deprecating and adding replacements for 5 functions:

  • std.os.windows.ntToWin32Namespace
  • std.os.windows.ReadLink
  • std.fs.Dir.readLinkW
  • std.posix.readLinkW
  • std.posix.readlinkatW

@squeek502
Copy link
Member

If the readLink changes are more involved, might be better to do those in a separate PR.

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 1bc5893 to 5ac4113 Compare April 26, 2025 13:11
Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

Great work, noticing/utilizing the ability to re-use the buffer for input/output of realpathW2 is a nice touch.

(the CI failure is unrelated to this PR, will re-run it once the CI server is restarted)

@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 5ac4113 to 2569835 Compare April 26, 2025 21:51
@mpfaff mpfaff marked this pull request as ready for review April 26, 2025 21:53
@mpfaff mpfaff force-pushed the realpathW-no-convert branch from 2569835 to fcf8123 Compare April 27, 2025 12:24
@squeek502
Copy link
Member

(no need to continually rebase btw, if the CI is all green then you can just wait for a core Zig team member to review it. If there's an unrelated failure on one or two CI runners, then those can be manually restarted without restarting all the rest of the runners [but all the runners need to have finished before the failing one(s) can be restarted])

@squeek502 squeek502 force-pushed the realpathW-no-convert branch from fcf8123 to 7bf740e Compare October 9, 2025 00:07
@squeek502 squeek502 enabled auto-merge October 9, 2025 00:08
@squeek502 squeek502 added release notes This PR should be mentioned in the release notes. standard library This issue involves writing Zig code for the standard library. labels Oct 9, 2025
@squeek502
Copy link
Member

squeek502 commented Oct 9, 2025

Release notes

realpathW functions and WTF-16

std.fs.Dir.realpathW, std.fs.realpathW, and std.posix.realpathW have been deprecated in favor of realpathW2 versions of each. The new versions fully deal with WTF-16 for both input and output, instead of taking in WTF-16 and outputting WTF-8. This opens up opportunities for more path buffer re-use, and therefore reduces stack usage of both realpathW itself (-64 KiB) and callsites (e.g. std.fs.selfExePath).

In the next release, the realpathW2 functions will be renamed to realpathW.

@squeek502 squeek502 merged commit 0bdd1b5 into ziglang:master Oct 9, 2025
17 of 18 checks passed
@squeek502
Copy link
Member

squeek502 commented Oct 9, 2025

@mpfaff thanks again for working on this, sorry for taking so long to get it merged. Let me know if you're still up for tackling readLinkW and friends; if not, I'll take a crack at it.

EDIT: #25539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes This PR should be mentioned in the release notes. standard library This issue involves writing Zig code for the standard library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants