[libcxx] Fix last_write_time tests for filesystems that don't support very small times.

[vsapsai]

Summary

APFS minimum supported file write time is -2^63 nanoseconds, which doesn't go
as far as file_time_type::min() that is equal to -2^63 microseconds on macOS.

This change doesn't affect filesystems that support file_time_type range only
for in-memory file time representation but not for on-disk representation. Such
filesystems are considered as SupportsMinTime.

rdar://problem/35865151

[vsapsai]

Don't like removing new_time = file_time_type::min() + MicroSec(1); part of the test_write_min_time but it escapes file_time_type::min() check. If somebody can explain the purpose of this test, I'd be glad to preserve it for filesystems that support very small times.

[Hahnfeld]

Can you please add a summary that describes which filesystem this problem can be seen with? I think outside people can't access rdar tickets, so I can only guess that it's related to APFS? Further guessing the filesystem supports negative timestamps in general but not file_time_type::min() (which probably is -2 ** 63)?

In D42755#993784, @vsapsai wrote:

Don't like removing new_time = file_time_type::min() + MicroSec(1); part of the test_write_min_time but it escapes file_time_type::min() check. If somebody can explain the purpose of this test, I'd be glad to preserve it for filesystems that support very small times.

I think the idea was to have something slightly larger than file_time_type::min() which generally makes sense for testing corner cases. However, there is no equivalent check for file_time_type::max() so I'd like to hear @EricWF whether we should keep it.

[vsapsai]

In D42755#994775, @Hahnfeld wrote:

Can you please add a summary that describes which filesystem this problem can be seen with? I think outside people can't access rdar tickets, so I can only guess that it's related to APFS? Further guessing the filesystem supports negative timestamps in general but not file_time_type::min() (which probably is -2 ** 63)?

Good suggestion for explaining the problem with file_time_type::min() in more detail. Also PR35990 has some details on how different filesystems handle file_time_type::min(). As the change doesn't fix the PR, I don't think it is worth mentioning it in the commit message but it can be still useful in code review discussion.

[Hahnfeld]

In D42755#995593, @vsapsai wrote:

Good suggestion for explaining the problem with file_time_type::min() in more detail. Also PR35990 has some details on how different filesystems handle file_time_type::min(). As the change doesn't fix the PR, I don't think it is worth mentioning it in the commit message but it can be still useful in code review discussion.

If there is an upstream bug report, please link to that one. With that information the changes look good, but I'd like @EricWF to accept.

[EricWF]

LGTM except the removal of the test. I think it's probably valuable to keep around on platforms that allow it.

What do you think?

libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

360 ↗

(On Diff #132233)

I would really love to keep this test. I think it has the ability to detect incorrect integer arithmetic in last_write_time when UBSAN is enabled.

Could we maybe add another check like SupportsAlmostMinTime where that checks ::min() + MicroSec(1)?

[vsapsai]

libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

360 ↗

(On Diff #132233)

What about nesting removed part in TimeIsRepresentableByFilesystem like

if (TimeIsRepresentableByFilesystem(new_time)) {
    TEST_CHECK(!ec);
    TEST_CHECK(tt >= new_time);
    TEST_CHECK(tt < new_time + Sec(1));

    ec = GetTestEC();
    last_write_time(p, Clock::now());

    new_time = file_time_type::min() + MicroSec(1);

    last_write_time(p, new_time, ec);
    tt = last_write_time(p);

    if (TimeIsRepresentableByFilesystem(new_time)) {
        TEST_CHECK(!ec);
        TEST_CHECK(tt >= new_time);
        TEST_CHECK(tt < new_time + Sec(1));
    }
}

As for me, I don't like how it looks. So maybe the same approach but with a flag like

bool supports_min_time = false;
if (TimeIsRepresentableByFilesystem(new_time)) {
    TEST_CHECK(!ec);
    TEST_CHECK(tt >= new_time);
    TEST_CHECK(tt < new_time + Sec(1));
    supports_min_time = true;
}

ec = GetTestEC();
last_write_time(p, Clock::now());

new_time = file_time_type::min() + MicroSec(1);

last_write_time(p, new_time, ec);
tt = last_write_time(p);

if (supports_min_time && TimeIsRepresentableByFilesystem(new_time)) {
    TEST_CHECK(!ec);
    TEST_CHECK(tt >= new_time);
    TEST_CHECK(tt < new_time + Sec(1));
}

Yet another option is to leverage that APFS truncates time to supported value, something like

if ((old_tt < new_time + Sec(1)) && TimeIsRepresentableByFilesystem(new_time))

I don't like it because it implicitly relies on time truncation which is not guaranteed for all filesystems and it is slightly harder to understand than boolean.

How do you like these ideas? SupportsAlmostMinTime can also work but I'd like to avoid repeating + MicroSec(1) part in 2 places.

[EricWF]

libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

360 ↗

(On Diff #132233)

The first option seems fine to me.

[vsapsai]

• Add back existing test but run it only when file system supports min time.

[vsapsai]

I am going to commit this change as I've addressed the review comments. If anybody has anything else to add, we can discuss that in post-commit review.

libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp

std::time_t old_write_time, new_write_time;
         old_write_time = LastWriteTime(file);
         file_time_type tp = file_time_type::max();
         fs::last_write_time(file, tp, ec);
         new_write_time = LastWriteTime(file);
     }
     return !ec && new_write_time > max_sec - 1;
 }
 
+bool TestSupportsMinTime() {
+    using namespace std::chrono;
+    using Lim = std::numeric_limits<std::time_t>;
+    auto min_sec = duration_cast<seconds>(file_time_type::min().time_since_epoch()).count();
+    if (min_sec < Lim::min()) return false;
+    std::error_code ec;
+    std::time_t old_write_time, new_write_time;
+    { // WARNING: Do not assert in this scope.
+      scoped_test_env env;
+      const path file = env.create_file("file", 42);
+      old_write_time = LastWriteTime(file);
+      file_time_type tp = file_time_type::min();
+      fs::last_write_time(file, tp, ec);
+      new_write_time = LastWriteTime(file);
+    }
+    return !ec && new_write_time < min_sec + 1;
+}
+
 #if defined(__clang__)
 #pragma clang diagnostic pop
 #endif
 
 static const bool SupportsNegativeTimes = TestSupportsNegativeTimes();
 static const bool SupportsMaxTime = TestSupportsMaxTime();
+static const bool SupportsMinTime = TestSupportsMinTime();
 
 } // end namespace
 
 // In some configurations, the comparison is tautological and the test is valid.
 // We disable the warning so that we can actually test it regardless. Also, that
 // diagnostic is pretty new, so also don't fail if old clang does not support it
 #if defined(__clang__)
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunknown-warning-option"
 #pragma clang diagnostic ignored "-Wunknown-pragmas"
 #pragma clang diagnostic ignored "-Wtautological-constant-compare"
 #endif
 
 // Check if a time point is representable on a given filesystem. Check that:
 // (A) 'tp' is representable as a time_t
 // (B) 'tp' is non-negative or the filesystem supports negative times.
 // (C) 'tp' is not 'file_time_type::max()' or the filesystem supports the max
 //     value.
+// (D) 'tp' is not 'file_time_type::min()' or the filesystem supports the min
+//     value.
 inline bool TimeIsRepresentableByFilesystem(file_time_type tp) {
     using namespace std::chrono;
     using Lim = std::numeric_limits<std::time_t>;
     auto sec = duration_cast<seconds>(tp.time_since_epoch()).count();
     auto microsec = duration_cast<microseconds>(tp.time_since_epoch()).count();
     if (sec < Lim::min() || sec > Lim::max())   return false;
     else if (microsec < 0 && !SupportsNegativeTimes) return false;
     else if (tp == file_time_type::max() && !SupportsMaxTime) return false;
+    else if (tp == file_time_type::min() && !SupportsMinTime) return false;
     return true;
 }
 
 #if defined(__clang__)
 #pragma clang diagnostic pop
 #endif
 
 TEST_SUITE(exists_test_suite)
TEST_CASE(test_write_min_time)
 
     last_write_time(p, new_time, ec);
     file_time_type tt = last_write_time(p);
 
     if (TimeIsRepresentableByFilesystem(new_time)) {
         TEST_CHECK(!ec);
         TEST_CHECK(tt >= new_time);
         TEST_CHECK(tt < new_time + Sec(1));
-    }
 
         ec = GetTestEC();
         last_write_time(p, Clock::now());
 
         new_time = file_time_type::min() + MicroSec(1);
 
         last_write_time(p, new_time, ec);
         tt = last_write_time(p);
 
         if (TimeIsRepresentableByFilesystem(new_time)) {
             TEST_CHECK(!ec);
             TEST_CHECK(tt >= new_time);
             TEST_CHECK(tt < new_time + Sec(1));
         }
     }
+}
 
 
 
 TEST_CASE(test_write_min_max_time)
 {
     using Clock = file_time_type::clock;
     using Sec = std::chrono::seconds;
     using Hours = std::chrono::hours;