[libc++] Fix 64-bit file creation for Bionic and Windows

[rprichard]

Summary

Bionic didn't add fopen64 until API 24, but there's no meaningful
distinction between them with Bionic, so just use fopen instead.

On Windows, use _chsize_s instead of _chsize. _chsize uses a 32-bit
long size argument, while _chsize_s uses __int64 instead.

Factor out utils::{off64_t, fopen64, ftruncate64} for use within the
create_file function.

[ldionne]

libcxx/test/support/filesystem_test_helper.h

218–223

I think it would be better to instead add utils::fopen64, utils::ftruncate64 and utils::off64_t. That way, we can simplify this code and hide the complexity inside utils:: above, which was the original goal.

[rprichard]

Factor out utils::{off64_t, fopen64, ftruncate64}.

[rprichard]

On Windows, use int64_t/_chsize_s instead of off_t/_chsize.

I'm not sure if MVS is actually 64-bit or not...

[ldionne]

Thank you. This makes the code simpler while supporting what you need. 🚢 it!

[rprichard]

Adding mstorsjo as subscriber in case they're interested in the Windows part of this change.

[mstorsjo]

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

[rprichard]

In D137132#3959285, @mstorsjo wrote:

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

_chsize uses a long size parameter, which is 32 bits on Windows, while _chsize_s uses __int64. Maybe I should add that to the commit comment.

[mstorsjo]

In D137132#3964968, @rprichard wrote:

In D137132#3959285, @mstorsjo wrote:

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

_chsize uses a long size parameter, which is 32 bits on Windows, while _chsize_s uses __int64. Maybe I should add that to the commit comment.

Oh, I see - thanks. That sounds reasonable yes.

Thanks for updating the commit message!

The patch is ok with me as long as CI keeps working (and the only failure there seems to be a fluke).

libcxx/test/support/filesystem_test_helper.h

 #else
 #include <io.h>
 #include <direct.h>
 #include <windows.h> // for CreateSymbolicLink, CreateHardLink
 #endif
 
 #include <cassert>
 #include <chrono>
+#include <cstdint>
 #include <cstdio> // for printf
 #include <string>
 #include <system_error>
 #include <vector>
 
 #include "make_string.h"
 #include "test_macros.h"
 #include "rapid-cxx-test.h"
 #include "format_string.h"
 
 // For creating socket files
 #if !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(_WIN32)
 # include <sys/socket.h>
 # include <sys/un.h>
 #endif
 
 namespace utils {
 #ifdef _WIN32
     inline int mkdir(const char* path, int mode) { (void)mode; return ::_mkdir(path); }
-    inline int ftruncate(int fd, off_t length) { return ::_chsize(fd, length); }
     inline int symlink(const char* oldname, const char* newname, bool is_dir) {
         DWORD flags = is_dir ? SYMBOLIC_LINK_FLAG_DIRECTORY : 0;
         if (CreateSymbolicLinkA(newname, oldname,
                                 flags | SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE))
           return 0;
         if (GetLastError() != ERROR_INVALID_PARAMETER)
           return 1;
         return !CreateSymbolicLinkA(newname, oldname, flags);
inline bool space(std::string path, std::uintmax_t &capacity,
         avail = FreeBytesAvailableToCaller.QuadPart;
         assert(capacity > 0);
         assert(free > 0);
         assert(avail > 0);
         return true;
     }
 #else
     using ::mkdir;
-    using ::ftruncate;
     inline int symlink(const char* oldname, const char* newname, bool is_dir) { (void)is_dir; return ::symlink(oldname, newname); }
     using ::link;
     using ::setenv;
     using ::unsetenv;
     inline bool space(std::string path, std::uintmax_t &capacity,
                       std::uintmax_t &free, std::uintmax_t &avail) {
         struct statvfs expect;
         if (::statvfs(path.c_str(), &expect) == -1)
inline bool space(std::string path, std::uintmax_t &capacity,
         };
         capacity = do_mult(expect.f_blocks);
         free = do_mult(expect.f_bfree);
         avail = do_mult(expect.f_bavail);
         return true;
     }
 #endif
 
+    // N.B. libc might define some of the foo[64] identifiers using macros from
+    // foo64 -> foo or vice versa.
+#if defined(_WIN32)
+    using off64_t = int64_t;
+#elif defined(__MVS__) || defined(__LP64__)
+    using off64_t = ::off_t;
+#else
+    using ::off64_t;
+#endif
+
+    inline FILE* fopen64(const char* pathname, const char* mode) {
+        // Bionic does not distinguish between fopen and fopen64, but fopen64
+        // wasn't added until API 24.
+#if defined(_WIN32) || defined(__MVS__) || defined(__LP64__) || defined(__BIONIC__)
+        return ::fopen(pathname, mode);
+#else
+        return ::fopen64(pathname, mode);
+#endif
+    }
+
+    inline int ftruncate64(int fd, off64_t length) {
+#if defined(_WIN32)
+        // _chsize_s sets errno on failure and also returns the error number.
+        return ::_chsize_s(fd, length) ? -1 : 0;
+#elif defined(__MVS__) || defined(__LP64__)
+        return ::ftruncate(fd, length);
+#else
+        return ::ftruncate64(fd, length);
+#endif
+    }
+
     inline std::string getcwd() {
         // Assume that path lengths are not greater than this.
         // This should be fine for testing purposes.
         char buf[4096];
         char* ret = ::getcwd(buf, sizeof(buf));
         assert(ret && "getcwd failed");
         return std::string(ret);
     }
#endif
     }
 
     // Purposefully using a size potentially larger than off_t here so we can
     // test the behavior of libc++fs when it is built with _FILE_OFFSET_BITS=64
     // but the caller is not (std::filesystem also uses uintmax_t rather than
     // off_t). On a 32-bit system this allows us to create a file larger than
     // 2GB.
     std::string create_file(fs::path filename_path, uintmax_t size = 0) {
-        std::string filename = filename_path.string();
-#if defined(__LP64__) || defined(_WIN32) || defined(__MVS__)
-        auto large_file_fopen = fopen;
-        auto large_file_ftruncate = utils::ftruncate;
-        using large_file_offset_t = off_t;
-#else
-        auto large_file_fopen = fopen64;
-        auto large_file_ftruncate = ftruncate64;
-        using large_file_offset_t = off64_t;
-#endif
-
-        filename = sanitize_path(std::move(filename));
+        std::string filename = sanitize_path(filename_path.string());
 
         if (size >
-            static_cast<typename std::make_unsigned<large_file_offset_t>::type>(
-                std::numeric_limits<large_file_offset_t>::max())) {
+            static_cast<typename std::make_unsigned<utils::off64_t>::type>(
+                std::numeric_limits<utils::off64_t>::max())) {
             fprintf(stderr, "create_file(%s, %ju) too large\n",
                     filename.c_str(), size);
             abort();
         }
 
 #if defined(_WIN32) || defined(__MVS__)
 #  define FOPEN_CLOEXEC_FLAG ""
 #else
 #  define FOPEN_CLOEXEC_FLAG "e"
 #endif
-        FILE* file = large_file_fopen(filename.c_str(), "w" FOPEN_CLOEXEC_FLAG);
+        FILE* file = utils::fopen64(filename.c_str(), "w" FOPEN_CLOEXEC_FLAG);
         if (file == nullptr) {
             fprintf(stderr, "fopen %s failed: %s\n", filename.c_str(),
                     strerror(errno));
             abort();
         }
 
-        if (large_file_ftruncate(
-                fileno(file), static_cast<large_file_offset_t>(size)) == -1) {
+        if (utils::ftruncate64(
+                fileno(file), static_cast<utils::off64_t>(size)) == -1) {
             fprintf(stderr, "ftruncate %s %ju failed: %s\n", filename.c_str(),
                     size, strerror(errno));
             fclose(file);
             abort();
         }
 
         fclose(file);
         return filename;