[libcxx-commits] [PATCH] D91141: [9/N] [libcxx] Implement the stat function family on top of native windows APIs

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:250-282
+#undef _S_IFMT
+#undef _S_IFDIR
+#undef _S_IFCHR
+#undef _S_IFIFO
+#undef _S_IFREG
+#undef _S_IFBLK
+#undef _S_IFLNK
----------------
ldionne wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > ldionne wrote:
> > > > Why are those defines necessary?
> > > Various versions of C runtimes used have fewer or more of them defined: to make things consistent across them, I undefine all existing defines and use our own.
> > I guess such a clarifying comment would help here.
> Just to understand, you're talking about the runtime headers for various versions of Windows? Is this not possible to fix by requiring a recent-ish version of those headers when building libc++?
No, it's more involved than that. There's two major toolchain styles for Windows; MSVC (the official, nonredistributable tools) and mingw (opensource, public-domain remake of windows headers). They target a number of different C runtime versions. (On Windows, the C runtime was traditionally not a system component - the system shipped with one legacy version, MSVC toolchains ship a newer one that you bundle with your app, and on Windows 10, there's both the legacy and the modern one shipped in the system).

So practically wrt this, it's not so much about requiring a new enough version - there's two entirely different toolchains that we need to support, and this particular area (stat functions, structs and related defines/macros) is differing annoyingly between the two.

Both of them provide constants _S_IFMT up to _S_IFREG. Neither of them provides _S_IFBLK, _S_IFLNK or _S_IFSOCK. Mingw does provide S_ISDIR up to S_ISREG, MSVC doesn't. Neither of them provide S_ISBLK, S_ISLNK or S_ISSOCK. 

We don't want to use the actual stat*() functions provided by the C runtimes (either version) because none of them really do what we want them to - so that's why I'd prefer to just flat out ignore the differing mess of what those toolchains provide in this context, as we provide our own implementation anyway.


================
Comment at: libcxx/src/filesystem/operations.cpp:402
+// (1601) to the Unix epoch (1970).
+#define FILE_TIME_OFFSET_SECS (uint64_t(369 * 365 + 89) * (24 * 60 * 60))
+
----------------
ldionne wrote:
> mstorsjo wrote:
> > ldionne wrote:
> > > Since the approach here is to make Windows into a POSIX-y system by adding `stat` & friends, would it be possible to hide this complexity in some internal header like `windows_posix_shims.h` (or whatever the name)?
> > Possibly, but there's a couple gotchas:
> > 
> > 1. Windows C runtimes actually do have a stat() function, but it's a bit limited (it only provides second precision for times, it doesn't expose the permissions, it doesn't provide something similar to st_dev), so we make our own available in the `detail` namespace here.
> > 
> > 2. Not all of these are exact replacements of posix functions. For symlinks (in D91143) I split the `symlink` function into `symlink_file` and `symlink_dir`.
> > 
> > 3. Like 2. above, we might want to tweak the interface of the functions outside of the posix interface. E.g. for the stat() function, it's currently implemented by a handful of API calls, to fill in all used fields. If a caller only wants some fields, we could add a flag parameter to `detail::stat` here, indicating exactly which fields we need populated, as a later optimization.
> > 
> I guess my actual ask is to properly define *some* cross-platform API that we call from this file. If it's not exactly POSIX, that's fine, however that should be documented.
> 
> We're porting the library to a new platform, so my preference would be that we delineate the minimal API that needs to be implemented by the system for the rest of the filesystem library to work. I'm not attached to whether we do it in this patch or later, but I think we need to do it somewhere in this patch list (I haven't looked at all of them, so if you do that later on, please LMK).
Well we do that, more or less, but implemented within operations.cpp within an anonymous namespace (so the code calls `detail::stat()` instead of `::stat()`). D91143 takes care of most of what is easily abstractable across platforms.

For functions that aren't quite as easy to abstract away (where the std::function we're implementing is the abstraction itself) I've gone with ifdefs within the individual functions, see e.g. D91170, where there's already ifdefs with two different codepaths within the function (for varying posix platforms).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91141/new/

https://reviews.llvm.org/D91141