Attach mtimes to individual deps, not files. Fixes a miscompile (!!)

Fix bug:

- Assume X and Y depend on Z
- X and Y both compile clean.
- A change is made to Z
- X and Y are now "out-of-date," and marked to compile.
- The change causes a failure in X, but Y still compiles clean.
- Because X compiles clean, the database will be updated and store the new mtime of Z!
- Attempting to compile again, the mtime of Z will not be considered "changed" since the last compile.
- X depends on Z, but Z is not marked "changed," and X will not be recompiled, even though it is still in a dirty state!
- Linking of X and Y produces wild results. UB. ORD NDR. Badness.

The fix is to have the edges in the graph between inputs and outputs
store the mtime of the input, rather than to store that mtime on the
file itself. Thus, each individual relationship between inputs and
outputs will track the "outdated-ness" of themselves.

This also adds a test which can correctly reproduce the issue in absense of the fix.

src/dds/build/deps.cpp

@@ -66,12 +66,11 @@ msvc_deps_info dds::parse_msvc_output_for_deps(std::string_view output, std::str
 }
 
 void dds::update_deps_info(database& db, const deps_info& deps) {
-    db.store_mtime(deps.output, fs::last_write_time(deps.output));
     db.store_file_command(deps.output, {deps.command, deps.command_output});
     db.forget_inputs_of(deps.output);
     for (auto&& inp : deps.inputs) {
-        db.store_mtime(inp, fs::last_write_time(inp));
-        db.record_dep(inp, deps.output);
+        auto mtime = fs::last_write_time(inp);
+        db.record_dep(inp, deps.output, mtime);
     }
 }
 
@@ -89,7 +88,7 @@ deps_rebuild_info dds::get_rebuild_info(database& db, path_ref output_path) {
     auto& inputs        = *inputs_;
     auto  changed_files =  //
         inputs             //
-        | ranges::views::filter([](const seen_file_info& input) {
+        | ranges::views::filter([](const input_file_info& input) {
               return !fs::exists(input.path) || fs::last_write_time(input.path) != input.last_mtime;
           })
         | ranges::views::transform([](auto& info) { return info.path; })  //

src/dds/build/plan/compile_exec.cpp

@@ -152,6 +152,7 @@ std::optional<deps_info> do_compile(const compile_file_full& cf, build_env_ref e
     }
 
     // We must always generate deps info if it was possible:
+    assert(compiled_okay);
     assert(ret_deps_info.has_value() || env.toolchain.deps_mode() == deps_mode::none);
     return ret_deps_info;
 }

src/dds/db/database.cpp

@@ -22,8 +22,17 @@ void migrate_1(sqlite3::database& db) {
     db.exec(R"(
         CREATE TABLE dds_files (
             file_id INTEGER PRIMARY KEY,
-            path TEXT NOT NULL UNIQUE,
-            mtime INTEGER NOT NULL
+            path TEXT NOT NULL UNIQUE
+        );
+        CREATE TABLE dds_file_commands (
+            command_id INTEGER PRIMARY KEY,
+            file_id
+                INTEGER
+                UNIQUE
+                NOT NULL
+                REFERENCES dds_files(file_id),
+            command TEXT NOT NULL,
+            output TEXT NOT NULL
         );
         CREATE TABLE dds_deps (
             input_file_id
@@ -34,18 +43,9 @@ void migrate_1(sqlite3::database& db) {
                 INTEGER
                 NOT NULL
                 REFERENCES dds_files(file_id),
+            input_mtime INTEGER NOT NULL,
             UNIQUE(input_file_id, output_file_id)
         );
-        CREATE TABLE dds_file_commands (
-            command_id INTEGER PRIMARY KEY,
-            file_id
-                INTEGER
-                UNIQUE
-                NOT NULL
-                REFERENCES dds_files(file_id),
-            command TEXT NOT NULL,
-            output TEXT NOT NULL
-        );
     )");
 }
 
@@ -106,72 +106,45 @@ database database::open(const std::string& db_path) {
 database::database(sqlite3::database db)
     : _db(std::move(db)) {}
 
-std::optional<fs::file_time_type> database::last_mtime_of(path_ref file_) {
+std::int64_t database::_record_file(path_ref path_) {
+    auto path = fs::weakly_canonical(path_);
+    sqlite3::exec(_stmt_cache(R"(
+                    INSERT OR IGNORE INTO dds_files (path)
+                    VALUES (?)
+                  )"_sql),
+                  std::forward_as_tuple(path.generic_string()));
     auto& st = _stmt_cache(R"(
-        SELECT mtime FROM dds_files WHERE path = ?
+        SELECT file_id
+          FROM dds_files
+         WHERE path = ?1
     )"_sql);
     st.reset();
-    auto path      = fs::weakly_canonical(file_);
-    st.bindings[1] = path.string();
-    auto maybe_res = sqlite3::unpack_single_opt<std::int64_t>(st);
-    if (!maybe_res) {
-        return std::nullopt;
-    }
-    auto [timestamp] = *maybe_res;
-    return fs::file_time_type(fs::file_time_type::duration(timestamp));
+    auto str       = path.generic_string();
+    st.bindings[1] = str;
+    auto [rowid]   = sqlite3::unpack_single<std::int64_t>(st);
+    return rowid;
 }
 
-void database::store_mtime(path_ref file, fs::file_time_type time) {
-    auto& st = _stmt_cache(R"(
-        INSERT INTO dds_files (path, mtime)
-        VALUES (?1, ?2)
-        ON CONFLICT(path) DO UPDATE SET mtime = ?2
+void database::record_dep(path_ref input, path_ref output, fs::file_time_type input_mtime) {
+    auto  in_id  = _record_file(input);
+    auto  out_id = _record_file(output);
+    auto& st     = _stmt_cache(R"(
+        INSERT OR IGNORE INTO dds_deps (input_file_id, output_file_id, input_mtime)
+        VALUES (?, ?, ?)
     )"_sql);
-    sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(file).string(),
-                                        time.time_since_epoch().count()));
-}
-
-void database::record_dep(path_ref input, path_ref output) {
-    auto& st = _stmt_cache(R"(
-        WITH input AS (
-            SELECT file_id
-              FROM dds_files
-             WHERE path = ?1
-        ),
-        output AS (
-            SELECT file_id
-              FROM dds_files
-             WHERE path = ?2
-        )
-        INSERT OR IGNORE INTO dds_deps (input_file_id, output_file_id)
-        VALUES (
-            (SELECT * FROM input),
-            (SELECT * FROM output)
-        )
-    )"_sql);
-    sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(input).string(),
-                                        fs::weakly_canonical(output).string()));
+    sqlite3::exec(st, std::forward_as_tuple(in_id, out_id, input_mtime.time_since_epoch().count()));
 }
 
 void database::store_file_command(path_ref file, const command_info& cmd) {
+    auto file_id = _record_file(file);
+
     auto& st = _stmt_cache(R"(
-        WITH file AS (
-            SELECT file_id
-            FROM dds_files
-            WHERE path = ?1
-        )
         INSERT OR REPLACE
           INTO dds_file_commands(file_id, command, output)
-        VALUES (
-            (SELECT * FROM file),
-            ?2,
-            ?3
-        )
+        VALUES (?1, ?2, ?3)
     )"_sql);
     sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(file).string(),
+                  std::forward_as_tuple(file_id,
                                         std::string_view(cmd.command),
                                         std::string_view(cmd.output)));
 }
@@ -189,31 +162,27 @@ void database::forget_inputs_of(path_ref file) {
     sqlite3::exec(st, std::forward_as_tuple(fs::weakly_canonical(file).string()));
 }
 
-std::optional<std::vector<seen_file_info>> database::inputs_of(path_ref file_) {
+std::optional<std::vector<input_file_info>> database::inputs_of(path_ref file_) {
     auto  file = fs::weakly_canonical(file_);
     auto& st   = _stmt_cache(R"(
         WITH file AS (
             SELECT file_id
               FROM dds_files
              WHERE path = ?
-        ),
-        input_ids AS (
-            SELECT input_file_id
-              FROM dds_deps
-             WHERE output_file_id IN file
         )
-        SELECT path, mtime
-          FROM dds_files
-         WHERE file_id IN input_ids
+        SELECT path, input_mtime
+          FROM dds_deps
+          JOIN dds_files ON input_file_id = file_id
+         WHERE output_file_id IN file
     )"_sql);
     st.reset();
     st.bindings[1] = file.string();
     auto tup_iter  = sqlite3::iter_tuples<std::string, std::int64_t>(st);
 
-    std::vector<seen_file_info> ret;
+    std::vector<input_file_info> ret;
     for (auto& [path, mtime] : tup_iter) {
         ret.emplace_back(
-            seen_file_info{path, fs::file_time_type(fs::file_time_type::duration(mtime))});
+            input_file_info{path, fs::file_time_type(fs::file_time_type::duration(mtime))});
     }
 
     if (ret.empty()) {

src/dds/db/database.hpp

@@ -20,7 +20,7 @@ struct command_info {
     std::string output;
 };
 
-struct seen_file_info {
+struct input_file_info {
     fs::path           path;
     fs::file_time_type last_mtime;
 };
@@ -33,6 +33,8 @@ class database {
     explicit database(neo::sqlite3::database db);
     database(const database&) = delete;
 
+    std::int64_t _record_file(path_ref p);
+
 public:
     static database open(const std::string& db_path);
     static database open(path_ref db_path) { return open(db_path.string()); }
@@ -43,13 +45,11 @@ public:
         return neo::sqlite3::transaction_guard(_db);
     }
 
-    std::optional<fs::file_time_type> last_mtime_of(path_ref file);
-    void                              store_mtime(path_ref file, fs::file_time_type time);
-    void                              record_dep(path_ref input, path_ref output);
-    void                              store_file_command(path_ref file, const command_info& cmd);
-    void                              forget_inputs_of(path_ref file);
+    void record_dep(path_ref input, path_ref output, fs::file_time_type input_mtime);
+    void store_file_command(path_ref file, const command_info& cmd);
+    void forget_inputs_of(path_ref file);
 
-    std::optional<std::vector<seen_file_info>> inputs_of(path_ref file);
+    std::optional<std::vector<input_file_info>> inputs_of(path_ref file);
     std::optional<command_info>                command_of(path_ref file);
 };
 

src/dds/db/database.test.cpp

@@ -5,17 +5,3 @@
 using namespace std::literals;
 
 TEST_CASE("Create a database") { auto db = dds::database::open(":memory:"s); }
-TEST_CASE("Read an absent file's mtime") {
-    auto db        = dds::database::open(":memory:"s);
-    auto mtime_opt = db.last_mtime_of("bad/file/path");
-    CHECK_FALSE(mtime_opt.has_value());
-}
-
-TEST_CASE("Record a file") {
-    auto db = dds::database::open(":memory:"s);
-    auto time = dds::fs::file_time_type::clock::now();
-    db.store_mtime("file/something", time);
-    auto mtime_opt = db.last_mtime_of("file/something");
-    REQUIRE(mtime_opt.has_value());
-    CHECK(mtime_opt == time);
-}

tests/db/project/src/1.cpp

@@ -0,0 +1,3 @@
+#include "./values.hpp"
+
+int value_1() { return first_value; }

tests/db/project/src/2.cpp

@@ -0,0 +1,3 @@
+#include "./values.hpp"
+
+int value_2() { return second_value; }

tests/db/project/src/app.main.cpp

@@ -0,0 +1,5 @@
+#include "./foo.hpp"
+
+#include <cmath>
+
+int main() { return std::abs(value_1() - value_2()); }

tests/db/project/src/foo.hpp

@@ -0,0 +1,4 @@
+#pragma once
+
+int value_1();
+int value_2();

tests/db/project/src/values.hpp

@@ -0,0 +1,4 @@
+#pragma once
+
+const int first_value  = 32;
+const int second_value = 32;

tests/db/test_compile_deps.py

@@ -0,0 +1,106 @@
+import subprocess
+import time
+
+import pytest
+
+from tests import dds, DDS, dds_fixture_conf_1
+from dds_ci import proc
+
+## #############################################################################
+## #############################################################################
+## The test project in this directory contains a single application and two
+## functions, each defined in a separate file. The two functions each return
+## an integer, and the application exit code will be the difference between
+## the two integers. (They are passed through std::abs(), so it is always a
+## positive integer). The default value is 32 in both functions.
+## #############################################################################
+## The purpose of these tests is to ensure the reliability of the compilation
+## dependency database. Having a miscompile because there was a failure to
+## detect file changes is a catastrophic bug!
+
+
+def build_and_get_rc(dds: DDS) -> int:
+    dds.build()
+    app = dds.build_dir / ('app' + dds.exe_suffix)
+    return proc.run(app).returncode
+
+
+def test_simple_rebuild(dds: DDS):
+    """
+    Check that changing a source file will update the resulting application.
+    """
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/1.cpp',
+            b'''
+            int value_1() { return 33; }
+            ''',
+        ))
+    # 33 - 32 = 1
+    assert build_and_get_rc(dds) == 1
+
+
+def test_rebuild_header_change(dds: DDS):
+    """Change the content of the header which defines the values"""
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/values.hpp',
+            b'''
+            const int first_value = 63;
+            const int second_value = 88;
+            ''',
+        ))
+    assert build_and_get_rc(dds) == (88 - 63)
+
+
+def test_partial_build_rebuild(dds: DDS):
+    """
+    Change the content of a header, but cause one user of that header to fail
+    compilation. The fact that compilation fails means it is still `out-of-date`,
+    and will need to be compiled after we have fixed it up.
+    """
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/values.hpp',
+            b'''
+            const int first_value_q  = 6;
+            const int second_value_q = 99;
+            ''',
+        ))
+    # Header now causes errors in 1.cpp and 2.cpp
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+    # Fix 1.cpp
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/1.cpp',
+            b'''
+            #include "./values.hpp"
+
+            int value_1() { return first_value_q; }
+            ''',
+        ))
+    # We will still see a failure, but now the DB will record the updated values.hpp
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+
+    # Should should raise _again_, even though we've successfully compiled one
+    # of the two files with the changed `values.hpp`, because `2.cpp` still
+    # has a pending update
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/2.cpp',
+            b'''
+            #include "./values.hpp"
+
+            int value_2() { return second_value_q; }
+            ''',
+        ))
+    # We should now compile and link to get the updated value
+    assert build_and_get_rc(dds) == (99 - 6)