From d402bc96297c6ed3dd6ee883dcff8cc39bd01030 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 18 Jan 2017 13:50:58 +0200 Subject: Ignore prerequisite mtimes that are not linker inputs This makes sure, for example, that we don't unnecessarily re-link an executable when its testscript prerequisite is changes. --- build2/algorithm | 48 ++++++++++++++++++++++++---------- build2/algorithm.cxx | 71 ++++++++++++--------------------------------------- build2/algorithm.ixx | 40 ++++++++++++++++++++++++----- build2/cc/compile.cxx | 25 +++++++++--------- build2/cc/link.cxx | 26 +++++++++++++------ build2/cli/rule.cxx | 14 +++++++--- build2/rule | 6 +---- build2/rule.cxx | 61 ++++++++++--------------------------------- build2/target | 15 +++++++++++ 9 files changed, 155 insertions(+), 151 deletions(-) (limited to 'build2') diff --git a/build2/algorithm b/build2/algorithm index 9401915..3dbef4c 100644 --- a/build2/algorithm +++ b/build2/algorithm @@ -180,15 +180,31 @@ namespace build2 target_state reverse_execute_prerequisites (action, target&); - // A version of the above that also determines whether the action - // needs to be executed on the target based on the passed mtime - // timestamp. + // A version of the above that also determines whether the action needs to + // be executed on the target based on the passed timestamp and filter. // - // Note that because we use mtime, this function should normally - // only be used in the perform_update action. + // The filter is passed each prerequisite target and is expected to signal + // which ones should be used for timestamp comparison. If the filter is + // NULL, then all the prerequisites are used. // - bool - execute_prerequisites (action, target&, const timestamp&); + // Note that the return value is a pair with the second half indicating + // whether any prerequisites were updated. This is used to handle the + // situation where some prerequisites were updated but no update of the + // target is necessary. In this case we still signal that the target was + // (conceptually, but not physically) changed. This is important both to + // propagate the fact that some work has been done and to also allow our + // dependents to detect this case if they are up to something tricky (like + // recursively linking liba{} prerequisites). + // + // Note that because we use mtime, this function should normally only be + // used in the perform_update action. + // + using prerequisite_filter = function; + + pair + execute_prerequisites (action, target&, + const timestamp&, + const prerequisite_filter& = nullptr); // Another version of the above that does two extra things for the caller: // it determines whether the action needs to be executed on the target based @@ -198,17 +214,23 @@ namespace build2 // prerequisites of the same type may get injected). // template - T* - execute_prerequisites (action, target&, const timestamp&); + pair + execute_prerequisites (action, target&, + const timestamp&, + const prerequisite_filter& = nullptr); - target* + pair execute_prerequisites (const target_type&, - action, target&, const timestamp&); + action, target&, + const timestamp&, + const prerequisite_filter& = nullptr); template - T* + pair execute_prerequisites (const target_type&, - action, target&, const timestamp&); + action, target&, + const timestamp&, + const prerequisite_filter& = nullptr); // Return noop_recipe instead of using this function directly. // diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index 638afd0..8874e96 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -413,54 +413,15 @@ namespace build2 return r; } - bool - execute_prerequisites (action a, target& t, const timestamp& mt) + pair + execute_prerequisites (const target_type* tt, + action a, target& t, + const timestamp& mt, const prerequisite_filter& pf) { bool e (mt == timestamp_nonexistent); - for (target* pt: t.prerequisite_targets) - { - if (pt == nullptr) // Skipped. - continue; - - target_state ts (execute (a, *pt)); - - if (!e) - { - // If this is an mtime-based target, then compare timestamps. - // - if (auto mpt = dynamic_cast (pt)) - { - timestamp mp (mpt->mtime ()); - - // What do we do if timestamps are equal? This can happen, for - // example, on filesystems that don't have subsecond resolution. - // There is not much we can do here except detect the case where - // the prerequisite was changed in this run which means the - // action must be executed on the target as well. - // - if (mt < mp || (mt == mp && ts == target_state::changed)) - e = true; - } - else - { - // Otherwise we assume the prerequisite is newer if it was changed. - // - if (ts == target_state::changed) - e = true; - } - } - } - - return e; - } - - target* - execute_prerequisites (const target_type& tt, - action a, target& t, const timestamp& mt) - { - bool e (mt == timestamp_nonexistent); - target* r (nullptr); + target* rt (tt != nullptr ? nullptr : &t); + target_state rs (target_state::unchanged); for (target* pt: t.prerequisite_targets) { @@ -468,8 +429,11 @@ namespace build2 continue; target_state ts (execute (a, *pt)); + rs |= ts; - if (!e) + // Should we compare the timestamp to this target's? + // + if (!e && (!pf || pf (*pt))) { // If this is an mtime-based target, then compare timestamps. // @@ -477,11 +441,8 @@ namespace build2 { timestamp mp (mpt->mtime ()); - // What do we do if timestamps are equal? This can happen, for - // example, on filesystems that don't have subsecond resolution. - // There is not much we can do here except detect the case where - // the prerequisite was changed in this run which means the - // action must be executed on the target as well. + // The same logic as in mtime_target::newer() (but avoids a call to + // state()). // if (mt < mp || (mt == mp && ts == target_state::changed)) e = true; @@ -495,12 +456,12 @@ namespace build2 } } - if (r == nullptr && pt->is_a (tt)) - r = pt; + if (rt == nullptr && pt->is_a (*tt)) + rt = pt; } - assert (r != nullptr); - return e ? r : nullptr; + assert (rt != nullptr); + return make_pair (e ? rt : nullptr, rs); } target_state diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx index 3128365..09d522f 100644 --- a/build2/algorithm.ixx +++ b/build2/algorithm.ixx @@ -217,18 +217,46 @@ namespace build2 } } + // If the first argument is NULL, then the result is treated as a boolean + // value. + // + pair + execute_prerequisites (const target_type*, + action, target&, + const timestamp&, const prerequisite_filter&); + + inline pair + execute_prerequisites (action a, target& t, + const timestamp& mt, const prerequisite_filter& pf) + { + auto p (execute_prerequisites (nullptr, a, t, mt, pf)); + return make_pair (p.first != nullptr, p.second); + } + template - inline T* - execute_prerequisites (action a, target& t, const timestamp& mt) + inline pair + execute_prerequisites (action a, target& t, + const timestamp& mt, const prerequisite_filter& pf) + { + auto p (execute_prerequisites (T::static_type, a, t, mt, pf)); + return make_pair (static_cast (p.first), p.second); + } + + inline pair + execute_prerequisites (const target_type& tt, + action a, target& t, + const timestamp& mt, const prerequisite_filter& pf) { - return static_cast (execute_prerequisites (T::static_type, a, t, mt)); + return execute_prerequisites (&tt, a, t, mt, pf); } template - inline T* + inline pair execute_prerequisites (const target_type& tt, - action a, target& t, const timestamp& mt) + action a, target& t, + const timestamp& mt, const prerequisite_filter& pf) { - return static_cast (execute_prerequisites (tt, a, t, mt)); + auto p (execute_prerequisites (tt, a, t, mt, pf)); + return make_pair (static_cast (p.first), p.second); } } diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx index 6fd64c2..3318f21 100644 --- a/build2/cc/compile.cxx +++ b/build2/cc/compile.cxx @@ -888,16 +888,7 @@ namespace build2 } } - if (ts != timestamp_unknown) - { - timestamp mt (pt.mtime ()); - - // See execute_prerequisites() for rationale behind the equal part. - // - return ts < mt || (ts == mt && pt.state () != target_state::changed); - } - - return false; + return ts != timestamp_unknown ? pt.newer (ts) : false; }; // Update and add a header file to the list of prerequisite targets. @@ -1394,10 +1385,18 @@ namespace build2 perform_update (action a, target& xt) const { file& t (static_cast (xt)); - file* s (execute_prerequisites (x_src, a, t, t.mtime ())); - if (s == nullptr) - return target_state::unchanged; + // Update prerequisites and determine if any relevant ones render us + // out-of-date. Note that currently we treat all the prerequisites + // as potentially affecting the result (for simplicity/performance). + // + file* s; + { + auto p (execute_prerequisites (x_src, a, t, t.mtime ())); + + if ((s = p.first) == nullptr) + return p.second; + } scope& bs (t.base_scope ()); scope& rs (*bs.root_scope ()); diff --git a/build2/cc/link.cxx b/build2/cc/link.cxx index 9dbfa36..a6b67d5 100644 --- a/build2/cc/link.cxx +++ b/build2/cc/link.cxx @@ -890,9 +890,12 @@ namespace build2 otype lt (link_type (t)); lorder lo (link_order (bs, lt)); - // Update prerequisites. + // Update prerequisites. We determine if any relevant ones render us + // out-of-date manually below. // - bool update (execute_prerequisites (a, t, t.mtime ())); + bool update (false); + timestamp mt (t.mtime ()); + target_state ts (execute_prerequisites (a, t)); // If targeting Windows, take care of the manifest. // @@ -913,7 +916,7 @@ namespace build2 t, rpath_timestamp != timestamp_nonexistent)); - timestamp mt (file_mtime (mf)); + timestamp mf_mt (file_mtime (mf)); if (tsys == "mingw32") { @@ -923,7 +926,7 @@ namespace build2 // manifest = mf + ".o"; - if (mt > file_mtime (manifest)) + if (mf_mt > file_mtime (manifest)) { path of (relative (manifest)); @@ -998,7 +1001,7 @@ namespace build2 { manifest = move (mf); // Save for link.exe's /MANIFESTINPUT. - if (mt > t.mtime ()) + if (mf_mt > mt) update = true; // Manifest changed, force update. } } @@ -1204,9 +1207,16 @@ namespace build2 else cs.append (f->path ().string ()); } + else + f = pt->is_a (); // Consider executable mtime (e.g., linker). + + // Check if this input renders us out-of-date. + // + if (f != nullptr) + update = update || f->newer (mt); } - // Treat it as input for both MinGW and VC. + // Treat it as input for both MinGW and VC (mtime checked above). // if (!manifest.empty ()) cs.append (manifest.string ()); @@ -1229,7 +1239,7 @@ namespace build2 // this situation in the "from scratch" flag. // bool scratch (false); - if (dd.writing () || dd.mtime () > t.mtime ()) + if (dd.writing () || dd.mtime () > mt) scratch = update = true; dd.close (); @@ -1237,7 +1247,7 @@ namespace build2 // If nothing changed, then we are done. // if (!update) - return target_state::unchanged; + return ts; // Ok, so we are updating. Finish building the command line. // diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx index 5b78723..b97bea2 100644 --- a/build2/cli/rule.cxx +++ b/build2/cli/rule.cxx @@ -231,12 +231,18 @@ namespace build2 { cli_cxx& t (static_cast (xt)); - // Execute our prerequsites and check if we are out of date. + // Update prerequisites and determine if any relevant ones render us + // out-of-date. Note that currently we treat all the prerequisites + // as potentially affecting the result (think prologues/epilogues, + // etc). // - cli* s (execute_prerequisites (a, t, t.mtime ())); + cli* s; + { + auto p (execute_prerequisites (a, t, t.mtime ())); - if (s == nullptr) - return target_state::unchanged; + if ((s = p.first) == nullptr) + return p.second; + } // Translate paths to relative (to working directory). This // results in easier to read diagnostics. diff --git a/build2/rule b/build2/rule index 8090fa0..015cf1a 100644 --- a/build2/rule +++ b/build2/rule @@ -39,8 +39,7 @@ namespace build2 apply (action, target&) const = 0; }; - // Fallback rule that on update verifies that the file exists and is - // not older than any of its prerequisites. + // Fallback rule that only matches if the file exists. // class file_rule: public rule { @@ -51,9 +50,6 @@ namespace build2 virtual recipe apply (action, target&) const override; - static target_state - perform_update (action, target&); - static file_rule instance; }; diff --git a/build2/rule.cxx b/build2/rule.cxx index a9b7c4e..1c0e08c 100644 --- a/build2/rule.cxx +++ b/build2/rule.cxx @@ -81,22 +81,18 @@ namespace build2 recipe file_rule:: apply (action a, target& t) const { - // Update triggers the update of this target's prerequisites - // so it would seem natural that we should also trigger their - // cleanup. However, this possibility is rather theoretical - // since such an update would render this target out of date - // which in turn would lead to an error. So until we see a - // real use-case for this functionality, we simply ignore - // the clean operation. + // Update triggers the update of this target's prerequisites so it would + // seem natural that we should also trigger their cleanup. However, this + // possibility is rather theoretical so until we see a real use-case for + // this functionality, we simply ignore the clean operation. // if (a.operation () == clean_id) return noop_recipe; - // If we have no prerequisites, then this means this file - // is up to date. Return noop_recipe which will also cause - // the target's state to be set to unchanged. This is an - // important optimization on which quite a few places that - // deal with predominantly static content rely. + // If we have no prerequisites, then this means this file is up to date. + // Return noop_recipe which will also cause the target's state to be set + // to unchanged. This is an important optimization on which quite a few + // places that deal with predominantly static content rely. // if (!t.has_prerequisites ()) return noop_recipe; @@ -104,43 +100,14 @@ namespace build2 // Search and match all the prerequisites. // search_and_match_prerequisites (a, t); - return a == perform_update_id ? &perform_update : default_recipe; - } - target_state file_rule:: - perform_update (action a, target& t) - { - // Make sure the target is not older than any of its prerequisites. + // Note that we used to provide perform_update() which checked that this + // target is not older than any of its prerequisites. However, later we + // realized this is probably wrong: consider a script with a testscript as + // a prerequisite; chances are the testscript will be newer than the + // script and there is nothing wrong with that. // - timestamp mt (dynamic_cast (t).mtime ()); - - for (target* pt: t.prerequisite_targets) - { - target_state ts (execute (a, *pt)); - - // If this is an mtime-based target, then compare timestamps. - // - if (auto mpt = dynamic_cast (pt)) - { - timestamp mp (mpt->mtime ()); - - if (mt < mp) - fail << "no recipe to " << diag_do (a, t) << - info << "prerequisite " << *pt << " is ahead of " << t - << " by " << (mp - mt); - } - else - { - // Otherwise we assume the prerequisite is newer if it was changed. - // - if (ts == target_state::changed) - fail << "no recipe to " << diag_do (a, t) << - info << "prerequisite " << *pt << " is ahead of " << t - << " because it was updated"; - } - } - - return target_state::unchanged; + return default_recipe; } file_rule file_rule::instance; diff --git a/build2/target b/build2/target index 1b75da3..dd9461c 100644 --- a/build2/target +++ b/build2/target @@ -1098,6 +1098,21 @@ namespace build2 mtime_ = mt; } + // Return true if this target is newer than the specified timestamp. + // + bool + newer (timestamp mt) + { + timestamp mp (mtime ()); + + // What do we do if timestamps are equal? This can happen, for example, + // on filesystems that don't have subsecond resolution. There is not + // much we can do here except detect the case where the target was + // changed on this run. + // + return mt < mp || (mt == mp && state () == target_state::changed); + } + protected: // Return timestamp_unknown if the mtime cannot be loaded. // -- cgit v1.1