[libcxx] Use _ftelli64/_fseeki64 on Windows #123128

mstorsjo · 2025-01-15T22:22:15Z

This allows using the full 64 bit range for file offsets.

This should fix the issue reported downstream at
mstorsjo/llvm-mingw#462.

llvmbot · 2025-01-15T22:22:49Z

@llvm/pr-subscribers-libcxx

Author: Martin Storsjö (mstorsjo)

Changes

This allows using the full 64 bit range for file offsets.

This should fix the issue reported downstream at
mstorsjo/llvm-mingw#462.

Full diff: https://github.com/llvm/llvm-project/pull/123128.diff

2 Files Affected:

(modified) libcxx/include/fstream (+9-2)
(modified) libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp (-6)

diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index f0e9425e0a53d9..b6f7f0439c566a 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -936,7 +936,11 @@ basic_filebuf<_CharT, _Traits>::seekoff(off_type __off, ios_base::seekdir __way,
   default:
     return pos_type(off_type(-1));
   }
-#    if !_LIBCPP_HAS_OFF_T_FUNCTIONS
+#    if defined(_LIBCPP_MSVCRT_LIKE)
+  if (_fseeki64(__file_, __width > 0 ? __width * __off : 0, __whence))
+    return pos_type(off_type(-1));
+  pos_type __r = _ftelli64(__file_);
+#    elif !_LIBCPP_HAS_OFF_T_FUNCTIONS
   if (fseek(__file_, __width > 0 ? __width * __off : 0, __whence))
     return pos_type(off_type(-1));
   pos_type __r = ftell(__file_);
@@ -954,7 +958,10 @@ typename basic_filebuf<_CharT, _Traits>::pos_type
 basic_filebuf<_CharT, _Traits>::seekpos(pos_type __sp, ios_base::openmode) {
   if (__file_ == nullptr || sync())
     return pos_type(off_type(-1));
-#    if !_LIBCPP_HAS_OFF_T_FUNCTIONS
+#    if defined(_LIBCPP_MSVCRT_LIKE)
+  if (_fseeki64(__file_, __sp, SEEK_SET))
+    return pos_type(off_type(-1));
+#    elif !_LIBCPP_HAS_OFF_T_FUNCTIONS
   if (fseek(__file_, __sp, SEEK_SET))
     return pos_type(off_type(-1));
 #    else
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp
index 5afd4465db31e0..ca6c609c7be312 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp
@@ -11,12 +11,6 @@
 // Test that we can seek using offsets larger than 32 bit, and that we can
 // retrieve file offsets larger than 32 bit.
 
-// On MSVC targets, we only use the 32 bit fseek/ftell functions. For MinGW
-// targets, we use fseeko/ftello, but the user needs to define
-// _FILE_OFFSET_BITS=64 to make them 64 bit.
-//
-// XFAIL: target={{.*}}-windows{{.*}}
-
 // On 32 bit Android platforms, off_t is 32 bit by default. By defining
 // _FILE_OFFSET_BITS=64, one gets a 64 bit off_t, but the corresponding
 // 64 bit ftello/fseeko functions are only available since Android API 24 (7.0).

mstorsjo · 2025-01-15T22:23:54Z

If doing this, I guess we should do the same change in include/__cxx03 too? Otherwise the test would fail on Windows if targeting C++03; we don't have that test config in CI though (all the C++03 tests are for other platforms), so it won't be caught.

philnik777 · 2025-01-28T08:39:15Z

libcxx/include/fstream

@@ -936,7 +936,11 @@ basic_filebuf<_CharT, _Traits>::seekoff(off_type __off, ios_base::seekdir __way,
  default:
    return pos_type(off_type(-1));
  }
-#    if !_LIBCPP_HAS_OFF_T_FUNCTIONS
+#    if defined(_LIBCPP_MSVCRT_LIKE)


I think it would be better if we refactor this to have an internal fseek instead of duplicating this multiple times. We also have something very similar in sync(), which you don't modify here.

Ok, I factorized this into __fseek and __ftell now and use them in these both locations, and in sync() which I had missed.

philnik777 · 2025-01-28T08:41:51Z

If doing this, I guess we should do the same change in include/__cxx03 too? Otherwise the test would fail on Windows if targeting C++03; we don't have that test config in CI though (all the C++03 tests are for other platforms), so it won't be caught.

For now there should just be an // UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME in the test if it doesn't work with the frozen headers.

mstorsjo · 2025-01-28T08:47:59Z

If doing this, I guess we should do the same change in include/__cxx03 too? Otherwise the test would fail on Windows if targeting C++03; we don't have that test config in CI though (all the C++03 tests are for other platforms), so it won't be caught.

For now there should just be an // UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME in the test if it doesn't work with the frozen headers.

Well the test does work with the frozen headers, but we don't get the 64 bit range on Windows until we apply the same fix as here, on the frozen headers.

philnik777 · 2025-01-28T09:34:06Z

If doing this, I guess we should do the same change in include/__cxx03 too? Otherwise the test would fail on Windows if targeting C++03; we don't have that test config in CI though (all the C++03 tests are for other platforms), so it won't be caught.

For now there should just be an // UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME in the test if it doesn't work with the frozen headers.

Well the test does work with the frozen headers, but we don't get the 64 bit range on Windows until we apply the same fix as here, on the frozen headers.

I meant "fails with the frozen headers".

mstorsjo · 2025-01-28T09:47:06Z

If doing this, I guess we should do the same change in include/__cxx03 too? Otherwise the test would fail on Windows if targeting C++03; we don't have that test config in CI though (all the C++03 tests are for other platforms), so it won't be caught.

For now there should just be an // UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME in the test if it doesn't work with the frozen headers.

Well the test does work with the frozen headers, but we don't get the 64 bit range on Windows until we apply the same fix as here, on the frozen headers.

I meant "fails with the frozen headers".

Right, although that would also exclude the test from running in C++03 mode with frozen headers on other OSes (where it works just fine). As we don't have CI coverage for that case combination (C++03 frozen headers on Windows), I think I'd rather either just let it slip (and fix the frozen headers later) or consider adding such a test configuration.

github-actions · 2025-01-28T12:02:33Z

✅ With the latest revision this PR passed the C/C++ code formatter.

philnik777 · 2025-01-28T22:28:41Z

libcxx/include/fstream

+  _LIBCPP_HIDE_FROM_ABI static int __fseek(FILE* __file, pos_type __offset, int __whence);
+  _LIBCPP_HIDE_FROM_ABI static pos_type __ftell(FILE* __file);


I think we can just make these free functions. (I'd just define them at the top of the file). It would also be good to remove the !defined(_LIBCPP_MSVCRT) from the definition of _LIBCPP_HAS_OFF_T_FUNCTIONS and probably just inline _LIBCPP_HAS_OFF_T_FUNCTIONS as a whole, since at that point it's just equivalent do defind(_NEWLIB_VERSION).

I kept the functions in the class, as they use pos_type, but I updated the PR to remove the unnecessary intermediate define _LIBCPP_HAS_OFF_T_FUNCTIONS.

philnik777

LGTM with green CI

This allows using the full 64 bit range for file offsets. This should fix the issue reported downstream at mstorsjo/llvm-mingw#462.

mstorsjo · 2025-01-29T13:26:18Z

/cherry-pick 86e20b0

llvmbot · 2025-01-29T13:31:56Z

/pull-request #124922

This allows using the full 64 bit range for file offsets. This should fix the issue reported downstream at mstorsjo/llvm-mingw#462. (cherry picked from commit 86e20b0)

mstorsjo requested a review from philnik777 January 15, 2025 22:22

mstorsjo requested a review from a team as a code owner January 15, 2025 22:22

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 15, 2025

philnik777 requested changes Jan 28, 2025

View reviewed changes

philnik777 reviewed Jan 28, 2025

View reviewed changes

philnik777 approved these changes Jan 29, 2025

View reviewed changes

mstorsjo added 5 commits January 29, 2025 10:50

[libcxx] Use _ftelli64/_fseeki64 on Windows

2739c3c

This allows using the full 64 bit range for file offsets. This should fix the issue reported downstream at mstorsjo/llvm-mingw#462.

Factorize repeated fseek/ftell calls

9b9f6c5

Fix formatting

26a0e91

Hide the new symbols from the ABI

a7e48af

Remove the intermediate _LIBCPP_HAS_OFF_T_FUNCTIONS define

b4be3ef

mstorsjo force-pushed the libcxx-tellg branch from 408297d to b4be3ef Compare January 29, 2025 08:50

mstorsjo merged commit 86e20b0 into llvm:main Jan 29, 2025
78 checks passed

mstorsjo added this to the LLVM 20.X Release milestone Jan 29, 2025

mstorsjo deleted the libcxx-tellg branch January 29, 2025 13:26

mstorsjo mentioned this pull request Feb 7, 2025

tellg returns -1 when using clang++ to get the size of a large file (around 6GB) on Windows mstorsjo/llvm-mingw#462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libcxx] Use _ftelli64/_fseeki64 on Windows #123128

[libcxx] Use _ftelli64/_fseeki64 on Windows #123128

Uh oh!

mstorsjo commented Jan 15, 2025

Uh oh!

llvmbot commented Jan 15, 2025

Uh oh!

mstorsjo commented Jan 15, 2025

Uh oh!

philnik777 Jan 28, 2025

Uh oh!

mstorsjo Jan 28, 2025

Uh oh!

philnik777 commented Jan 28, 2025

Uh oh!

mstorsjo commented Jan 28, 2025

Uh oh!

philnik777 commented Jan 28, 2025

Uh oh!

mstorsjo commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025 •

edited

Loading

Uh oh!

philnik777 Jan 28, 2025

Uh oh!

mstorsjo Jan 29, 2025

Uh oh!

philnik777 left a comment

Uh oh!

Uh oh!

mstorsjo commented Jan 29, 2025

Uh oh!

llvmbot commented Jan 29, 2025

Uh oh!

Uh oh!

		_LIBCPP_HIDE_FROM_ABI static int __fseek(FILE* __file, pos_type __offset, int __whence);
		_LIBCPP_HIDE_FROM_ABI static pos_type __ftell(FILE* __file);

[libcxx] Use _ftelli64/_fseeki64 on Windows #123128

[libcxx] Use _ftelli64/_fseeki64 on Windows #123128

Uh oh!

Conversation

mstorsjo commented Jan 15, 2025

Uh oh!

llvmbot commented Jan 15, 2025

Uh oh!

mstorsjo commented Jan 15, 2025

Uh oh!

philnik777 Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

mstorsjo Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 commented Jan 28, 2025

Uh oh!

mstorsjo commented Jan 28, 2025

Uh oh!

philnik777 commented Jan 28, 2025

Uh oh!

mstorsjo commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philnik777 Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

mstorsjo Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mstorsjo commented Jan 29, 2025

Uh oh!

llvmbot commented Jan 29, 2025

Uh oh!

Uh oh!

github-actions bot commented Jan 28, 2025 •

edited

Loading