aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2021-12-16 13:37:09 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2021-12-16 13:37:09 +0200
commitdab9482b868e2da40d04210c0443bf4d2dcfdfd5 (patch)
tree7d7e95df261433611ed3a954ca9c1f43afe3b7a3
parent5c3744e914d72916d30c9b4cb4804227d6aae736 (diff)
Verify targets that alias same path are read-only
-rw-r--r--libbuild2/operation.cxx114
-rw-r--r--libbuild2/target.hxx7
-rw-r--r--libbuild2/target.ixx4
3 files changed, 121 insertions, 4 deletions
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index 8ef1a13..0f30c4a 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -3,7 +3,8 @@
#include <libbuild2/operation.hxx>
-#include <iostream> // cout
+#include <iostream> // cout
+#include <unordered_map>
#include <libbuild2/file.hxx>
#include <libbuild2/scope.hxx>
@@ -127,6 +128,111 @@ namespace build2
ts.push_back (t);
}
+ // Verify that no two targets share a path unless they both are "read-only"
+ // (have noop recipes).
+ //
+ // Note: somewhat similar logic in dyndep::verify_existing_file().
+ //
+ static void
+ verify_targets (context& ctx, action a)
+ {
+ // On the first pass we collect all the targets that have non-noop
+ // recipes. On the second pass we check if there are any other targets
+ // that have the same path. Note that we must also deal with two non-noop
+ // targets that have the same path.
+ //
+ // Strictly speaking we may need to produce some sort of progress if this
+ // takes long. However, currently we are looking at verification speed of
+ // ~1ms per 2K targets, which means it will only becomes noticeable with
+ // over 1M targets.
+ //
+ unordered_map<reference_wrapper<const path>,
+ const target*,
+ hash<path>,
+ equal_to<path>> map;
+
+ // Half of the total appears to be a reasonable heuristics.
+ //
+ map.reserve (ctx.targets.size () / 2);
+
+ bool e (false);
+
+ for (size_t pass (1); pass != 3; ++pass)
+ {
+ for (const auto& pt: ctx.targets)
+ {
+ // We are only interested in path-based targets.
+ //
+ const path_target* t (pt->is_a<path_target> ());
+ if (t == nullptr)
+ continue;
+
+ // We are only interested in the matched targets.
+ //
+ const target::opstate& s (t->state[a]);
+
+ if (s.task_count.load (memory_order_relaxed) - ctx.count_base () <
+ target::offset_matched)
+ continue;
+
+ // Skip if for some reason the path is not assigned.
+ //
+ const path& p (t->path (memory_order_relaxed));
+ if (p.empty ())
+ continue;
+
+ recipe_function* const* rf (s.recipe.target<recipe_function*> ());
+ bool noop (rf != nullptr && *rf == &noop_action);
+
+ if ((noop ? 2 : 1) != pass)
+ continue;
+
+ const target* t1;
+ if (pass == 1)
+ {
+ auto r (map.emplace (p, t));
+
+ if (r.second)
+ continue;
+
+ t1 = r.first->second;
+ }
+ else
+ {
+ auto i (map.find (p));
+
+ if (i == map.end ())
+ continue;
+
+ t1 = i->second;
+ }
+
+ e = true;
+
+ diag_record dr (error);
+
+ dr << "multiple targets share path " << p <<
+ info << "first target: " << *t1 <<
+ info << "second target: " << *t <<
+ info << "target " << *t1 << " has non-noop recipe";
+
+ if (pass == 1)
+ {
+ dr << info << "target " << *t << " has non-noop recipe";
+ }
+ else if (t->decl != target_decl::real)
+ {
+ dr << info << "target " << *t << " is not explicitly declared "
+ << "in any buildfile" <<
+ info << "perhaps it is a dynamic dependency?";
+ }
+ }
+ }
+
+ if (e)
+ throw failed ();
+ }
+
void
match (const values&, action a, action_targets& ts, uint16_t diag, bool prog)
{
@@ -248,6 +354,12 @@ namespace build2
if (fail)
throw failed ();
+
+ // @@ This feels a bit ad hoc. Maybe we should invent operation hooks
+ // for this (e.g., post-search, post-match, post-execute)?
+ //
+ if (a == perform_update_id)
+ verify_targets (ctx, a);
}
// Phase restored to load.
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index c2df3e7..4ce871b 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -1489,6 +1489,9 @@ namespace build2
iterator begin () const {return map_.begin ();}
iterator end () const {return map_.end ();}
+ size_t
+ size () const {return map_.size ();}
+
void
clear () {map_.clear ();}
@@ -1601,6 +1604,8 @@ namespace build2
using path_type = build2::path;
+ // Target path. Must be absolute and normalized.
+ //
// Target path is an "atomic consistent cash". That is, it can be set at
// any time (including on a const instance) but any subsequent updates
// must set the same path. Or, in other words, once the path is set, it
@@ -1630,7 +1635,7 @@ namespace build2
// the path_mtime() function to do it in the correct order.
//
const path_type&
- path () const;
+ path (memory_order = memory_order_acquire) const;
const path_type&
path (path_type) const;
diff --git a/libbuild2/target.ixx b/libbuild2/target.ixx
index 50750ca..540c718 100644
--- a/libbuild2/target.ixx
+++ b/libbuild2/target.ixx
@@ -582,13 +582,13 @@ namespace build2
// path_target
//
inline const path& path_target::
- path () const
+ path (memory_order mo) const
{
// You may be wondering why don't we spin the transition out? The reason
// is it shouldn't matter since were we called just a moment earlier, we
// wouldn't have seen it.
//
- return path_state_.load (memory_order_acquire) == 2 ? path_ : empty_path;
+ return path_state_.load (mo) == 2 ? path_ : empty_path;
}
inline const path& path_target::