From ac5ef35b5fa8b938ed427df4b0ad44e5b6b52cff Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Mon, 19 Nov 2018 14:58:20 +0200
Subject: Finalize workaround for backwards modification time issue

---
 build2/cc/compile-rule.cxx | 10 +++++-----
 build2/cc/link-rule.cxx    | 25 ++++++++++++------------
 build2/cli/rule.cxx        |  2 +-
 build2/depdb.cxx           | 37 ++++++++++++++++++++++++++---------
 build2/depdb.hxx           | 48 ++++++++++++++++++++--------------------------
 build2/in/rule.cxx         |  2 +-
 6 files changed, 69 insertions(+), 55 deletions(-)

diff --git a/build2/cc/compile-rule.cxx b/build2/cc/compile-rule.cxx
index 634a586..e361e9d 100644
--- a/build2/cc/compile-rule.cxx
+++ b/build2/cc/compile-rule.cxx
@@ -751,8 +751,7 @@ namespace build2
         // Note: the leading '@' is reserved for the module map prefix (see
         // extract_modules()) and no other line must start with it.
         //
-        md.dd = tp + ".d";
-        depdb dd (md.dd);
+        depdb dd (tp + ".d");
 
         // First should come the rule name/version.
         //
@@ -824,7 +823,7 @@ namespace build2
         // the target (interrupted update), then do unconditional update.
         //
         timestamp mt;
-        bool u (dd.writing () || dd.mtime () > (mt = file_mtime (tp)));
+        bool u (dd.writing () || dd.mtime > (mt = file_mtime (tp)));
         if (u)
           mt = timestamp_nonexistent; // Treat as if it doesn't exist.
 
@@ -996,9 +995,10 @@ namespace build2
         // we will keep re-validating the cached data over and over again.
         //
         if (u && dd.reading ())
-          dd.touch ();
+          dd.touch = true;
 
         dd.close ();
+        md.dd = move (dd.path);
 
         // If the preprocessed output is suitable for compilation, then pass
         // it along.
@@ -1036,7 +1036,7 @@ namespace build2
         // store the first in the target file (so we do touch it) and the
         // second in depdb (which is never newer that the target).
         //
-        md.mt = u ? timestamp_nonexistent : dd.mtime ();
+        md.mt = u ? timestamp_nonexistent : dd.mtime;
       }
 
       switch (a)
diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx
index 90d290d..77619d1 100644
--- a/build2/cc/link-rule.cxx
+++ b/build2/cc/link-rule.cxx
@@ -2068,11 +2068,14 @@ namespace build2
       // this situation in the "from scratch" flag.
       //
       bool scratch (false);
-      if (dd.writing () || dd.mtime () > mt)
+      if (dd.writing () || dd.mtime > mt)
         scratch = update = true;
 
+#define MTIME_SANITY_CHECK
+#ifdef MTIME_SANITY_CHECK
       timestamp dd_tt (system_clock::now ());
-      bool      dd_rd (dd.reading ());
+#endif
+
       dd.close ();
 
       // If nothing changed, then we are done.
@@ -2080,9 +2083,6 @@ namespace build2
       if (!update)
         return ts;
 
-      path ddp (tp + ".d");
-      timestamp dd_mt (file_mtime (ddp));
-
       // Ok, so we are updating. Finish building the command line.
       //
       string in, out, out1, out2, out3; // Storage.
@@ -2544,19 +2544,20 @@ namespace build2
 
       rm.cancel ();
 
+#ifdef MTIME_SANITY_CHECK
       {
-        timestamp tp_tt (system_clock::now ());
         timestamp tp_mt (file_mtime (tp));
+        timestamp dd_mt (file_mtime (dd.path));
+        timestamp tp_tt (system_clock::now ());
 
         if (dd_mt > tp_mt)
-        {
           fail << "backwards modification times:\n"
-               << ddp.string () <<   " " << dd_mt << " (" << dd_rd << ")\n"
-               << tp.string () << "   " << tp_mt << '\n'
-               << dd_tt << '\n'
-               << tp_tt;
-        }
+               << dd_tt << " window start\n"
+               << dd_mt << " " << dd.path.string () << '\n'
+               << tp_mt << " " << tp.string () << '\n'
+               << tp_tt << " window end";
       }
+#endif
 
       // Should we go to the filesystem and get the new mtime? We know the
       // file has been modified, so instead just use the current clock time.
diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx
index 27668c1..6c87490 100644
--- a/build2/cli/rule.cxx
+++ b/build2/cli/rule.cxx
@@ -262,7 +262,7 @@ namespace build2
 
         // Update if depdb mismatch.
         //
-        if (dd.writing () || dd.mtime () > mt)
+        if (dd.writing () || dd.mtime > mt)
           update = true;
 
         dd.close ();
diff --git a/build2/depdb.cxx b/build2/depdb.cxx
index df73dfd..416f1f4 100644
--- a/build2/depdb.cxx
+++ b/build2/depdb.cxx
@@ -14,15 +14,15 @@ using namespace butl;
 namespace build2
 {
   depdb::
-  depdb (const path& f)
-      : mtime_ (file_mtime (f)), touch_ (false)
+  depdb (path_type p)
+      : path (move (p)), mtime (file_mtime (path)), touch (false)
   {
     fstream::openmode om (fstream::out | fstream::binary);
     fstream::iostate em (fstream::badbit);
 
-    if (mtime_ == timestamp_nonexistent)
+    if (mtime == timestamp_nonexistent)
     {
-      mtime_ = timestamp_unknown;
+      mtime = timestamp_unknown;
       state_ = state::write;
       em |= fstream::failbit;
     }
@@ -32,13 +32,13 @@ namespace build2
       om |= fstream::in;
     }
 
-    fs_.open (f.string (), om);
+    fs_.open (path.string (), om);
     if (!fs_.is_open ())
     {
       bool c (state_ == state::write);
 
       diag_record dr (fail);
-      dr << "unable to " << (c ? "create" : "open") << ' ' << f;
+      dr << "unable to " << (c ? "create" : "open") << ' ' << path;
 
       if (c)
         dr << info << "did you forget to add fsdir{} prerequisite for "
@@ -93,7 +93,7 @@ namespace build2
     fs_.seekp (pos_); // Must be done when changing from read to write.
 
     state_ = state::write;
-    mtime_ = timestamp_unknown;
+    mtime = timestamp_unknown;
   }
 
   string* depdb::
@@ -217,15 +217,17 @@ namespace build2
       // and skip updating mtime (which would probably be incorrect).
       //
       // It would be interesting to one day write an implementation that uses
-      // POSIX file OI, futimens(), and ftruncate() and see how much better it
+      // POSIX file IO, futimens(), and ftruncate() and see how much better it
       // performs.
       //
-      if (touch_)
+      if (touch)
       {
         fs_.clear ();
         fs_.exceptions (fstream::failbit | fstream::badbit);
         fs_.seekp (0, fstream::cur); // Required to switch from read to write.
         fs_.put ('\0');
+
+        state_ = state::write; // See below.
       }
     }
     else
@@ -246,5 +248,22 @@ namespace build2
     }
 
     fs_.close ();
+
+    // On some platforms (currently confirmed on Windows and FreeBSD, both
+    // running as VMs) one can sometimes end up with a modification time that
+    // is quite a bit after the call to close(). And this messes with our
+    // arrangement that a valid depdb should be no older than the target it
+    // is for.
+    //
+    // Note that this does not seem to be related to clock adjustments but
+    // rather feels like the modification time is set when the changes
+    // actually hit some lower-level layer (e.g., OS or filesystem driver).
+    // One workaround that appears to work is to query the mtime. This seems
+    // to force that layer to commit to a timestamp.
+    //
+#if defined(_WIN32) || defined(__FreeBSD__)
+    if (state_ == state::write)
+      file_mtime (path);
+#endif
   }
 }
diff --git a/build2/depdb.hxx b/build2/depdb.hxx
index 9885723..7c9e464 100644
--- a/build2/depdb.hxx
+++ b/build2/depdb.hxx
@@ -61,6 +61,19 @@ namespace build2
   class depdb
   {
   public:
+    using path_type = build2::path;
+
+    // The modification time of the database only makes sense while reading
+    // (in the write mode it will be set to timestamp_unknown).
+    //
+    // If touch is set to true, update the database modification time in
+    // close() even if otherwise no modifications are necessary (i.e., the
+    // database is in the read mode and is at eof).
+    //
+    path_type path;
+    timestamp mtime;
+    bool      touch;
+
     // Open the database for reading. Note that if the file does not exist,
     // has wrong format version, or is corrupt, then the database will be
     // immediately switched to writing.
@@ -71,20 +84,7 @@ namespace build2
     // prerequisite. Handling this as io_error in every rule that uses depdb
     // would be burdensome thus we issue the diagnostics here.
     //
-    depdb (const path&);
-
-    // Return the modification time of the database. This value only makes
-    // sense while reading (in the write mode it will be timestamp_unknown).
-    //
-    timestamp
-    mtime () const {return mtime_;}
-
-    // Update the database modification time in close() even if otherwise
-    // no modifications are necessary (i.e., the database is in the read
-    // mode and is at eof).
-    //
-    void
-    touch () {touch_ = true;}
+    depdb (path_type);
 
     // Close the database. If this function is not called, then the database
     // may be left in the old/currupt state. Note that in the read mode this
@@ -121,9 +121,6 @@ namespace build2
     bool
     writing () const {return state_ == state::write;}
 
-    bool
-    touched () const {return touch_;}
-
     // Skip to the end of the database and return true if it is valid.
     // Otherwise, return false, in which case the database must be
     // overwritten. Note that this function expects the database to be in the
@@ -140,7 +137,7 @@ namespace build2
     write (const string& l, bool nl = true) {write (l.c_str (), l.size (), nl);}
 
     void
-    write (const path& p, bool nl = true) {write (p.string (), nl);}
+    write (const path_type& p, bool nl = true) {write (p.string (), nl);}
 
     void
     write (const char* s, bool nl = true) {write (s, std::strlen (s), nl);}
@@ -178,10 +175,10 @@ namespace build2
     }
 
     string*
-    expect (const path& v)
+    expect (const path_type& v)
     {
       string* l (read ());
-      if (l == nullptr || path::traits::compare (*l, v.string ()) != 0)
+      if (l == nullptr || path_type::traits::compare (*l, v.string ()) != 0)
       {
         write (v);
         return l;
@@ -211,14 +208,11 @@ namespace build2
     read_ ();
 
   private:
-    timestamp mtime_;
-    std::fstream fs_;
-
-    std::fstream::pos_type pos_; // Start of the last returned line.
-    string line_;
-
     enum class state {read, read_eof, write} state_;
-    bool touch_;
+
+    std::fstream           fs_;
+    std::fstream::pos_type pos_;  // Start of the last returned line.
+    string                 line_; // Current line.
   };
 }
 
diff --git a/build2/in/rule.cxx b/build2/in/rule.cxx
index 450f398..86b8ffa 100644
--- a/build2/in/rule.cxx
+++ b/build2/in/rule.cxx
@@ -157,7 +157,7 @@ namespace build2
 
       // Update if any mismatch or depdb is newer that the output.
       //
-      if (dd.writing () || dd.mtime () > mt)
+      if (dd.writing () || dd.mtime > mt)
         update = true;
 
       // Substituted variable values.
-- 
cgit v1.1