[libcxx] [test] Add a test for the range of file offsets #122798

mstorsjo · 2025-01-13T21:44:33Z

This adds a test for an issue reported downstream at mstorsjo/llvm-mingw#462; this is known to fail on Windows right now, where the fseek/ftell calls end up truncated to 32 bits.

The test for this, unfortunately, requires temporarily creating a 4 GB file.

This adds a test for an issue reported downstream at mstorsjo/llvm-mingw#462; this is known to fail on Windows right now, where the fseek/ftell calls end up truncated to 32 bits. The test for this, unfortunately, requires temporarily creating a 4 GB file.

llvmbot · 2025-01-13T21:45:06Z

@llvm/pr-subscribers-libcxx

Author: Martin Storsjö (mstorsjo)

Changes

This adds a test for an issue reported downstream at mstorsjo/llvm-mingw#462; this is known to fail on Windows right now, where the fseek/ftell calls end up truncated to 32 bits.

The test for this, unfortunately, requires temporarily creating a 4 GB file.

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

1 Files Affected:

(added) libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp (+67)

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
new file mode 100644
index 00000000000000..99675ff7e3c81b
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp
@@ -0,0 +1,67 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <fstream>
+
+// XFAIL: target={{.*}}-windows{{.*}}
+
+#include <fstream>
+#include <iostream>
+#include <cassert>
+#include <vector>
+
+#include "assert_macros.h"
+#include "platform_support.h"
+#include "test_macros.h"
+
+void test_tellg(std::streamoff total_size) {
+  std::vector<char> data(8192);
+  for (std::size_t i = 0; i < data.size(); ++i)
+    data[i] = static_cast<char>(i % (1 << 8 * sizeof(char)));
+  std::string p = get_temp_file_name();
+  {
+    std::ofstream ofs;
+    ofs.open(p, std::ios::out | std::ios::binary);
+    assert(ofs.is_open());
+    for (std::streamoff size = 0; size < total_size;) {
+      std::size_t n = std::min(static_cast<std::streamoff>(data.size()), total_size - size);
+      ofs.write(data.data(), n);
+      size += n;
+    }
+    assert(!ofs.fail());
+    ofs.close();
+  }
+  {
+    std::ifstream ifs;
+    ifs.open(p, std::ios::binary);
+    assert(ifs.is_open());
+    std::streamoff in_off = ifs.tellg();
+    TEST_REQUIRE(in_off == 0, [&] { test_eprintf("in_off = %ld\n", in_off); });
+    ifs.seekg(total_size - 20, std::ios::beg);
+    in_off = ifs.tellg();
+    TEST_REQUIRE(in_off == total_size - 20, [&] {
+      test_eprintf("ref = %zu, in_off = %ld\n", total_size - 20, in_off);
+    });
+    ifs.seekg(10, std::ios::cur);
+    in_off = ifs.tellg();
+    TEST_REQUIRE(in_off == total_size - 10, [&] {
+      test_eprintf("ref = %zu, in_off = %ld\n", total_size - 10, in_off);
+    });
+    ifs.seekg(0, std::ios::end);
+    in_off = ifs.tellg();
+    TEST_REQUIRE(in_off == total_size, [&] { test_eprintf("ref = %zu, in_off = %ld\n", total_size, in_off); });
+  }
+  std::remove(p.c_str());
+}
+
+int main(int, char**) {
+  // TODO: What if std::streamoff is only 32 bit, which may be the case on
+  // some platforms?
+  test_tellg(0x100000042ULL);
+  return 0;
+}

mstorsjo · 2025-01-13T21:50:36Z

I have a fix suggestion for the issue on Windows too - I can either file that PR right away if you want to, on top of this one (with 2 commits), or wait until this one is settled before filing the second one.

philnik777 · 2025-01-13T22:00:14Z

libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp

+  // TODO: What if std::streamoff is only 32 bit, which may be the case on
+  // some platforms?


Maybe just static_assert(sizeof(std::streamoff) > 4) and almost certainly // UNSUPPORTED: 32-bit-pointer?

Right, such a static assert probably would make sense.

No, 32 bit platforms are fine, you can clearly operate on files larger than 4 GB even if running a 32 bit process, and libc++ does define std::streamoff as a 64 bit data type even in 32 bit mode - at least on 32 bit Windows where I've tested this.

The static_assert() is making the tests fail in C++03 mode:

# .---command stderr------------ # | /__w/llvm-project/llvm-project/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp:68:3: error: 'static_assert' with no message is a C++17 extension [-Werror,-Wc++17-extensions] # | 68 | static_assert(sizeof(std::streamoff) > 4); # | | ^ # | /__w/llvm-project/llvm-project/build/generic-cxx03/libcxx/test-suite-install/include/c++/v1/__config:307:58: note: expanded from macro 'static_assert' # | 307 | # define static_assert(...) _Static_assert(__VA_ARGS__) # | | ^ # | 1 error generated. # `-----------------------------

Any suggestion on which way to go about it?

Just make it static_assert(condition, ""). That's what we usually do pre-C++17.

Oh, I see - doh. Thank you!

philnik777

LGTM with comments addressed.

philnik777 · 2025-01-13T22:29:55Z

libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp

+}
+
+int main(int, char**) {
+  static_assert(sizeof(std::streamoff) > 4);


I think it would be good to add a comment that this an assumption of the test, not that std::streamoff actually has to be larger than four bytes.

philnik777 · 2025-01-13T22:30:36Z

libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp

+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+


Could you add a short comment what you're testing?

The macOS C++03 test configuration disallows lambdas in C++03.

mstorsjo · 2025-01-14T12:24:55Z

From a previous CI run https://buildkite.com/llvm-project/libcxx-ci/builds/39993, I see that this test seems to fail on the android-ndk-21-def-x86, CC @rprichard. From the output log about where it failed, Assertion failure: in_off == total_size - 20, it would look to me like this is the same issue as on Windows, presumably affecting either all 32 bit versions of Android or targets that are old enough.

I'm also seeing one failure on AIX 32 bit, Assertion failed: !ofs.fail(), which would indicate that it failed to write the output file - either because it is out of diskspace, or because AIX 32 bit doesn't allow writing files that are >4 GB in size, CC @daltenty. Can you confirm whether this is expected to fail on 32 bit AIX, or whether it would be a flaky test (if the runner some times has less than 4 GB of space, but not always)?

mstorsjo · 2025-01-15T22:03:20Z

From a previous CI run https://buildkite.com/llvm-project/libcxx-ci/builds/39993, I see that this test seems to fail on the android-ndk-21-def-x86, CC @rprichard. From the output log about where it failed, Assertion failure: in_off == total_size - 20, it would look to me like this is the same issue as on Windows, presumably affecting either all 32 bit versions of Android or targets that are old enough.

I'm also seeing one failure on AIX 32 bit, Assertion failed: !ofs.fail(), which would indicate that it failed to write the output file - either because it is out of diskspace, or because AIX 32 bit doesn't allow writing files that are >4 GB in size, CC @daltenty. Can you confirm whether this is expected to fail on 32 bit AIX, or whether it would be a flaky test (if the runner some times has less than 4 GB of space, but not always)?

FWIW, I added XFAILs for these cases, and this is all green through CI now, so I'll go ahead and merge it.

mstorsjo · 2025-01-16T09:42:30Z

@kazutakahirata This test seems to fail on the llvm-clang-win-x-armv7l buildbot, which it seems that you maintain: https://lab.llvm.org/buildbot/#/builders/38/builds/1855

The test fails like this:

offset_range.pass.cpp:54: void test_tellg(std::streamoff): Assertion `!ofs.fail()' failed.

This probably means that the runner didn't have space to create the 4 GB temporary file used in this test.

We have a libcxx testsuite flag large_tests, so one can set up the testsuite to run e.g. like -DLIBCXX_TEST_PARAMS="large_tests=False", which should exclude tests that require a large amount of memory to run. I wonder if we'd need to add a similar flag here? (We have large_tests for tests that require a lot of memory, and long_tests for tests that take a long time to run - but requiring a lot of space doesn't really fit either of them, even if you'd probably be setting many of these flags in this kind of configuration.)

vvereschaka · 2025-01-16T23:19:08Z

@mstorsjo ,

@kazutakahirata This test seems to fail on the llvm-clang-win-x-armv7l buildbot, which it seems that you maintain: https://lab.llvm.org/buildbot/#/builders/38/builds/1855

The test fails like this:
offset_range.pass.cpp:54: void test_tellg(std::streamoff): Assertion `!ofs.fail()' failed.
This probably means that the runner didn't have space to create the 4 GB temporary file used in this test.

This is cross-platform builder Win to Armv7 Linux. The tests is running on a armv7 (32 bit) development board with a limited disk space (8 Gb total). I freed 5.5G on the dev board, but anyway the test gets failed with the ``!ofs.fail()' failed` error. Looks like the problem is not only a missing free 4Gb space.

We have a libcxx testsuite flag large_tests, so one can set up the testsuite to run e.g. like - DLIBCXX_TEST_PARAMS="large_tests=False", which should exclude tests that require a large amount of memory to run. I wonder if we'd need to add a similar flag here?

Would be helpful I suppose. The test takes 78 secs on the board even if it gets failed (single thread)

********************
Slowest Tests:
--------------------------------------------------------------------------
76.53s: llvm-libc++-static.cfg.in :: std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp

and it takes 270 secs on Aarch64 board (32 threads, Jetson AGX) when passed:

Slowest Tests:
--------------------------------------------------------------------------
268.93s: llvm-libc++-static.cfg.in :: std/input.output/file.streams/fstreams/ifstream.members/offset_range.pass.cpp
97.36s: llvm-libc++-static.cfg.in :: std/numerics/rand/rand.dist/rand.dist.bern/rand.dist.bern.negbin/eval.pass.cpp
89.58s: llvm-libc++-static.cfg.in :: std/utilities/variant/variant.visit/visit_return_type.pass.cpp
...

https://lab.llvm.org/buildbot/#/builders/193/builds/4935/steps/15/logs/stdio

…ets. XFAIL offset_range test on the ARMv7 targets (32-bit). Ref PR llvm#122798

…x targets. (#123449) Mark the `offset_range` test as UNSUPPORTED for the `armv7-unknown-linux-gnueabihf` target (32-bit). Ref PR #122798

mstorsjo requested a review from a team as a code owner January 13, 2025 21:44

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

philnik777 reviewed Jan 13, 2025

View reviewed changes

Remove the TODO comment, add a static assert.

cc0bbb3

philnik777 approved these changes Jan 13, 2025

View reviewed changes

mstorsjo added 3 commits January 14, 2025 00:41

Add comments about the purpose of the test, and about the static assert.

a943081

Try to fix the static_assert() for pre-C++17

4f202ce

Don't use lambdas in TEST_REQUIRE

1683503

The macOS C++03 test configuration disallows lambdas in C++03.

Add more XFAILs and comments about them.

a1c8550

mstorsjo merged commit 8de51c8 into llvm:main Jan 15, 2025
77 checks passed

mstorsjo deleted the libcxx-seekg-tellg-range-test branch January 15, 2025 22:03

vvereschaka added a commit to vvereschaka/llvm-project that referenced this pull request Jan 18, 2025

[libcxx][test] Fix a test for the range of file offsets on ARMv7 targ…

f850478

…ets. XFAIL offset_range test on the ARMv7 targets (32-bit). Ref PR llvm#122798

vvereschaka mentioned this pull request Jan 18, 2025

[libcxx][test] Fix a test for the range of file offsets on ARMv7 Linux targets. #123449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libcxx] [test] Add a test for the range of file offsets #122798

[libcxx] [test] Add a test for the range of file offsets #122798

mstorsjo commented Jan 13, 2025

llvmbot commented Jan 13, 2025

mstorsjo commented Jan 13, 2025

philnik777 Jan 13, 2025

mstorsjo Jan 13, 2025

mstorsjo Jan 14, 2025

philnik777 Jan 14, 2025

mstorsjo Jan 14, 2025

philnik777 left a comment

philnik777 Jan 13, 2025

philnik777 Jan 13, 2025

mstorsjo commented Jan 14, 2025

mstorsjo commented Jan 15, 2025

mstorsjo commented Jan 16, 2025

vvereschaka commented Jan 16, 2025

		// TODO: What if std::streamoff is only 32 bit, which may be the case on
		// some platforms?

[libcxx] [test] Add a test for the range of file offsets #122798

[libcxx] [test] Add a test for the range of file offsets #122798

Conversation

mstorsjo commented Jan 13, 2025

llvmbot commented Jan 13, 2025

mstorsjo commented Jan 13, 2025

philnik777 Jan 13, 2025

Choose a reason for hiding this comment

mstorsjo Jan 13, 2025

Choose a reason for hiding this comment

mstorsjo Jan 14, 2025

Choose a reason for hiding this comment

philnik777 Jan 14, 2025

Choose a reason for hiding this comment

mstorsjo Jan 14, 2025

Choose a reason for hiding this comment

philnik777 left a comment

Choose a reason for hiding this comment

philnik777 Jan 13, 2025

Choose a reason for hiding this comment

philnik777 Jan 13, 2025

Choose a reason for hiding this comment

mstorsjo commented Jan 14, 2025

mstorsjo commented Jan 15, 2025

mstorsjo commented Jan 16, 2025

vvereschaka commented Jan 16, 2025