From 33111a31562f364e496b8c8bba47693babecdbc0 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 15 May 2019 07:57:45 +0200 Subject: Add ability to depend on (declared) ad hoc group member --- build2/algorithm.cxx | 107 ++++++++++++++++++++++++++++++++++----------- build2/algorithm.hxx | 47 +++++++++++++------- build2/algorithm.ixx | 80 ++++++++++++++++++++++----------- build2/cc/compile-rule.cxx | 6 ++- build2/cc/link-rule.cxx | 55 ++++++++++++----------- build2/parser.cxx | 32 ++++++++++---- build2/search.cxx | 10 +++-- build2/target.hxx | 20 +++++---- 8 files changed, 242 insertions(+), 115 deletions(-) diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index bcdccf6..2e3fe83 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -206,33 +206,34 @@ namespace build2 sched.resume (task_count); } - target_lock - add_adhoc_member (action a, - target& t, + target& + add_adhoc_member (target& t, const target_type& tt, const dir_path& dir, const dir_path& out, - const string& n) + string n) { + tracer trace ("add_adhoc_member"); + const_ptr* mp (&t.member); for (; *mp != nullptr && !(*mp)->is_a (tt); mp = &(*mp)->member) ; - const target& m (*mp != nullptr // Might already be there. - ? **mp - : search (t, tt, dir, out, n)); - - target_lock l (lock (a, m)); - assert (l.target != nullptr); // Someone messing with ad hoc members? - + target& m (*mp != nullptr // Might already be there. + ? **mp + : targets.insert (tt, + dir, + out, + move (n), + nullopt /* ext */, + true /* implied */, + trace).first); if (*mp == nullptr) { - *mp = l.target; - l.target->group = &t; + *mp = &m; + m.group = &t; } - else - assert ((*mp)->dir == dir && (*mp)->name == n); // Basic sanity check. - return l; + return m; }; // Return the matching rule or NULL if no match and try_match is true. @@ -522,22 +523,62 @@ namespace build2 ? optional (scheduler::work_none) : nullopt)); - if (l.target == nullptr) - { - // Already applied, executed, or busy. - // - if (l.offset >= target::offset_busy) - return make_pair (true, target_state::busy); - - // Fall through. - } - else + if (l.target != nullptr) { assert (l.offset < target::offset_applied); // Shouldn't lock otherwise. if (try_match && l.offset == target::offset_tried) return make_pair (false, target_state::unknown); + // Handle matching ad hoc group member. + // + if (ct.adhoc_member ()) + { + const target& g (*ct.group); + + // It feels natural to "convert" this call to the one for the group, + // including the try_match and async parts. However, that async part + // is tricky: we will be called again to finish the match (the else- + // block below) where we need to perform the equivalent conversion. + // Semantically, we want to achieve the following: + // + // match (a, g); | match_async (a, g); + // | match (a, g); + // match_recipe (l, group_recipe); | match_recipe (l, group_recipe); + // + // We also have to "unstack" this lock to avoid racing with stack + // modifications by the asynchronous match (see below). Since an ad + // hoc member doesn't have any prerequisites of its own (and thus + // cannot depend on the group), skipping this link during the cycle + // detection feels harmless (note that the other way around is + // possible and still works). + // + l.unstack (); + + +#if 0 // The same story as with the lock stack. I think the only way to + // make this work is to somehow pass the member down to match_impl(). + auto df = make_diag_frame ( + [a, &ct](const diag_record& dr) + { + if (verb != 0) + dr << info << "while matching rule group to " << diag_do (a, ct); + }); +#endif + + auto r (match (a, g, start_count, task_count, try_match)); + + if (r.first) + { + match_inc_dependens (a, g); + match_recipe (l, group_recipe); + } + else + l.offset = target::offset_tried; + + return r; // Group state. + } + if (task_count == nullptr) return match_impl (l, false /* step */, try_match); @@ -579,6 +620,20 @@ namespace build2 // Matched synchronously, fall through. } + else + { + // Already applied, executed, or busy. + // + if (l.offset >= target::offset_busy) + return make_pair (true, target_state::busy); + + // Handle matching ad hoc group member (the finish part; see above). + // + if (ct.adhoc_member ()) + return match (a, *ct.group, 0, nullptr); + + // Fall through. + } return ct.try_matched_state (a, false); } diff --git a/build2/algorithm.hxx b/build2/algorithm.hxx index f1aad0c..e3473ad 100644 --- a/build2/algorithm.hxx +++ b/build2/algorithm.hxx @@ -155,6 +155,9 @@ namespace build2 const target_lock* stack; // Tip of the stack. const target_lock* prev; + void + unstack (); + struct stack_guard { explicit stack_guard (const target_lock* s): s_ (stack) {stack = s;} @@ -175,37 +178,49 @@ namespace build2 // action. // // @@ MT fuzzy: what if it is already in the desired state, why assert? - // Currently we only use it with match_recipe(). + // Currently we only use it with match_recipe() and if it is matched + // but not applied, then it's not clear why we are overriding that + // match. // target_lock lock (action, const target&); // Add an ad hoc member to the end of the chain assuming that an already - // existing member of this target type is the same. Return the locked member - // target. - // - target_lock - add_adhoc_member (action, - target&, + // existing member of this target type is the same. Return the newly added + // or already existing target. The member directories (dir and out) are + // expected to be absolute and normalized. + // + // Note that here and in find_adhoc_member() below we use target type (as + // opposed to, say, type and name) as the member's identity. This fits our + // current needs where every (rule-managed) ad hoc member has a unique + // target type and allows us to support things like overriding the ad hoc + // member name by the user. + // + target& + add_adhoc_member (target&, const target_type&, const dir_path& dir, const dir_path& out, - const string& name); + string name); // If the extension is specified then it is added to the member's target // name. // - target_lock - add_adhoc_member (action, - target&, - const target_type&, - const char* ext = nullptr); + target& + add_adhoc_member (target&, const target_type&, const char* ext = nullptr); + + template + inline T& + add_adhoc_member (target& g, const target_type& tt, const char* e = nullptr) + { + return static_cast (add_adhoc_member (g, tt, e)); + } template - inline target_lock - add_adhoc_member (action a, target& g, const char* e = nullptr) + inline T& + add_adhoc_member (target& g, const char* e = nullptr) { - return add_adhoc_member (a, g, T::static_type, e); + return add_adhoc_member (g, T::static_type, e); } // Find an ad hoc member of the specified target type returning NULL if not diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx index 0150587..c79ee49 100644 --- a/build2/algorithm.ixx +++ b/build2/algorithm.ixx @@ -135,14 +135,29 @@ namespace build2 } inline void target_lock:: + unstack () + { + if (target != nullptr && prev != this) + { + assert (stack == this); + stack = prev; + prev = this; + } + } + + inline void target_lock:: unlock () { if (target != nullptr) { unlock_impl (action, *target, offset); - assert (stack == this); - stack = prev; + if (prev != this) + { + assert (stack == this); + stack = prev; + } + target = nullptr; } } @@ -154,8 +169,12 @@ namespace build2 if (target != nullptr) { - assert (stack == this); - stack = prev; + if (prev != this) + { + assert (stack == this); + stack = prev; + } + target = nullptr; } @@ -174,9 +193,14 @@ namespace build2 { if (target != nullptr) { - assert (stack == &x); - prev = x.prev; - stack = this; + if (x.prev != &x) + { + assert (stack == &x); + prev = x.prev; + stack = this; + } + else + prev = this; x.target = nullptr; } @@ -195,9 +219,14 @@ namespace build2 if (target != nullptr) { - assert (stack == &x); - prev = x.prev; - stack = this; + if (x.prev != &x) + { + assert (stack == &x); + prev = x.prev; + stack = this; + } + else + prev = this; x.target = nullptr; } @@ -232,8 +261,8 @@ namespace build2 return r; } - inline target_lock - add_adhoc_member (action a, target& t, const target_type& tt, const char* e) + inline target& + add_adhoc_member (target& t, const target_type& tt, const char* e) { string n (t.name); @@ -243,7 +272,7 @@ namespace build2 n += e; } - return add_adhoc_member (a, t, tt, t.dir, t.out, n); + return add_adhoc_member (t, tt, t.dir, t.out, move (n)); } inline target* @@ -271,6 +300,13 @@ namespace build2 pair match (action, const target&, size_t, atomic_count*, bool try_match = false); + inline void + match_inc_dependens (action a, const target& t) + { + dependency_count.fetch_add (1, memory_order_relaxed); + t[a].dependents.fetch_add (1, memory_order_release); + } + inline target_state match (action a, const target& t, bool fail) { @@ -279,10 +315,7 @@ namespace build2 target_state r (match (a, t, 0, nullptr).second); if (r != target_state::failed) - { - dependency_count.fetch_add (1, memory_order_relaxed); - t[a].dependents.fetch_add (1, memory_order_release); - } + match_inc_dependens (a, t); else if (fail) throw failed (); @@ -300,10 +333,7 @@ namespace build2 if (r.first) { if (r.second != target_state::failed) - { - dependency_count.fetch_add (1, memory_order_relaxed); - t[a].dependents.fetch_add (1, memory_order_release); - } + match_inc_dependens (a, t); else if (fail) throw failed (); } @@ -333,7 +363,9 @@ namespace build2 } case unmatch::safe: { - // Safe if unchanged or someone else is also a dependent. + // Safe if unchanged or someone else is also a dependent (note that + // we never decrement this count during match so that someone else + // cannot change their mind). // if (s == target_state::unchanged || t[a].dependents.load (memory_order_consume) != 0) @@ -343,9 +375,7 @@ namespace build2 } } - dependency_count.fetch_add (1, memory_order_relaxed); - t[a].dependents.fetch_add (1, memory_order_release); - + match_inc_dependens (a, t); return false; } diff --git a/build2/cc/compile-rule.cxx b/build2/cc/compile-rule.cxx index cd49689..3458a1b 100644 --- a/build2/cc/compile-rule.cxx +++ b/build2/cc/compile-rule.cxx @@ -677,8 +677,10 @@ namespace build2 // (e.g., foo.mxx and foo.cxx) which means obj*{} targets could // collide. So we add the module extension to the target name. // - target_lock obj (add_adhoc_member (a, t, tts.obj, e.c_str ())); - obj.target->as ().derive_path (o); + file& obj (add_adhoc_member (t, tts.obj, e.c_str ())); + + if (obj.path ().empty ()) + obj.derive_path (o); } } diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx index dd34d95..94e19e0 100644 --- a/build2/cc/link-rule.cxx +++ b/build2/cc/link-rule.cxx @@ -291,8 +291,12 @@ namespace build2 lk = b; append_ext (lk); - libi& li (*find_adhoc_member (ls)); // Note: libi is locked. - lk = li.derive_path (move (lk), tsys == "mingw32" ? "a" : "lib"); + libi& li (*find_adhoc_member (ls)); + const path& pi (li.path ()); + + lk = pi.empty () + ? li.derive_path (move (lk), tsys == "mingw32" ? "a" : "lib") + : pi; } else if (!v.empty ()) { @@ -600,8 +604,6 @@ namespace build2 // to not derive the path for the library target itself inside. // { - target_lock libi; // Have to hold until after PDB member addition. - const char* e (nullptr); // Extension. const char* p (nullptr); // Prefix. const char* s (nullptr); // Suffix. @@ -715,8 +717,8 @@ namespace build2 // if (tclass == "windows") { - libi = add_adhoc_member (a, t); e = "dll"; + add_adhoc_member (t); } md.libs_data = derive_libs_paths (t, p, s); @@ -734,17 +736,19 @@ namespace build2 if (find_option ("/DEBUG", t, c_loptions, true) || find_option ("/DEBUG", t, x_loptions, true)) { + const target_type& tt (*bs.find_target_type ("pdb")); + // We call the target foo.{exe,dll}.pdb rather than just foo.pdb // because we can have both foo.exe and foo.dll in the same // directory. // - target_lock pdb ( - add_adhoc_member (a, t, *bs.find_target_type ("pdb"), e)); + file& pdb (add_adhoc_member (t, tt, e)); // Note that the path is derived from the exe/dll path (so it // will include the version in case of a dll). // - pdb.target->as ().derive_path (t.path (), "pdb"); + if (pdb.path ().empty ()) + pdb.derive_path (t.path (), "pdb"); } } @@ -759,20 +763,19 @@ namespace build2 // if (ot != otype::e) { - target_lock pc ( - add_adhoc_member ( - a, t, - ot == otype::a ? pca::static_type : pcs::static_type)); + file& pc (add_adhoc_member (t, + (ot == otype::a + ? pca::static_type + : pcs::static_type))); // Note that here we always use the lib name prefix, even on // Windows with VC. The reason is the user needs a consistent name // across platforms by which they can refer to the library. This - // is also the reason why we use the .static/.shared second-level - // extensions rather that a./.lib/.so/.dylib/.dll. + // is also the reason why we use the .static and .shared second- + // level extensions rather that a./.lib and .so/.dylib/.dll. // - pc.target->as ().derive_path (nullptr, - (p == nullptr ? "lib" : p), - s); + if (pc.path ().empty ()) + pc.derive_path (nullptr, (p == nullptr ? "lib" : p), s); } // Add the Windows rpath emulating assembly directory as fsdir{}. @@ -791,14 +794,14 @@ namespace build2 // exists (windows_rpath_assembly() does take care to clean it up // if not used). // - target_lock dir ( - add_adhoc_member ( - a, - t, - fsdir::static_type, - path_cast (t.path () + ".dlls"), - t.out, - string ())); +#ifdef _WIN32 + target& dir = +#endif + add_adhoc_member (t, + fsdir::static_type, + path_cast (t.path () + ".dlls"), + t.out, + string () /* name */); // By default our backlinking logic will try to symlink the // directory and it can even be done on Windows using junctions. @@ -812,7 +815,7 @@ namespace build2 // Windows. // #ifdef _WIN32 - dir.target->state[a].assign (var_backlink) = "copy"; + dir.state[a].assign (var_backlink) = "copy"; #endif } } diff --git a/build2/parser.cxx b/build2/parser.cxx index 06be54a..12dcd54 100644 --- a/build2/parser.cxx +++ b/build2/parser.cxx @@ -973,6 +973,24 @@ namespace build2 if (n.qualified ()) fail (loc) << "project name in target " << n; + // We derive the path unless the target name ends with the '...' escape + // which here we treat as the "let the rule derive the path" indicator + // (see target::split_name() for details). This will only be useful for + // referring to ad hoc members that are managed by the group's matching + // rule. Note also that omitting '...' for such a member could be used + // to override the file name, provided the rule checks if the path has + // already been derived before doing it itself. + // + bool escaped; + { + const string& v (n.value); + size_t p (v.size ()); + + escaped = (p > 3 && + v[--p] == '.' && v[--p] == '.' && v[--p] == '.' && + v[--p] != '.'); + } + target& at ( enter_target::insert_target (*this, move (n), move (o), @@ -985,8 +1003,6 @@ namespace build2 // Add as an ad hoc member at the end of the chain skipping duplicates. // { - // @@ ADHOC: call add_adhoc_member()? - // const_ptr* mp (&target_->member); for (; *mp != nullptr; mp = &(*mp)->member) { @@ -1002,15 +1018,13 @@ namespace build2 *mp = &at; at.group = target_; } - else - continue; // Duplicate. } - // @@ ADHOC: What if it's something like .pdb where the group derives a - // custom extension... Hm... - // - if (file* ft = at.is_a ()) - ft->derive_path (); + if (!escaped) + { + if (file* ft = at.is_a ()) + ft->derive_path (); + } } } diff --git a/build2/search.cxx b/build2/search.cxx index 7683ef6..68fd5a5 100644 --- a/build2/search.cxx +++ b/build2/search.cxx @@ -228,9 +228,13 @@ namespace build2 // // @@ OUT: same story as in search_existing_target() re out. // - auto r ( - targets.insert ( - *tk.type, move (d), *tk.out, *tk.name, tk.ext, true, trace)); + auto r (targets.insert (*tk.type, + move (d), + *tk.out, + *tk.name, + tk.ext, + true /* implied */, + trace)); const target& t (r.first); l5 ([&]{trace << (r.second ? "new" : "existing") << " target " << t diff --git a/build2/target.hxx b/build2/target.hxx index e167211..4c0efbe 100644 --- a/build2/target.hxx +++ b/build2/target.hxx @@ -1539,14 +1539,18 @@ namespace build2 // any time but any subsequent updates must set the same path. Or, in // other words, once the path is set, it never changes. // - // A set empty path may signify special unknown/undetermined/unreal - // location (for example, a binless library or an installed import library - // -- we know the DLL is there, just not exactly where). In this case you - // would also normally set its mtime. We used to return a pointer to - // properly distinguish between not set and empty but that proved too - // tedious. Note that this means there could be a race between path and - // mtime (unless you lock the target in some other way; see file_rule) so - // for this case it makes sense to set the timestamp first. + // An empty path may signify special unknown/undetermined/unreal location + // (for example, a binless library or an installed import library -- we + // know the DLL is there, just not exactly where). In this case you would + // also normally set its mtime. + // + // We used to return a pointer to properly distinguish between not set and + // empty but that proved too tedious to work with. So now we return empty + // path both when not set (which will be empty_path so you can distinguish + // the two case if you really want to) and when set to empty. Note that + // this means there could be a race between path and mtime (unless you + // lock the target in some other way; see file_rule) so in this case it + // makes sense to set the timestamp first. // const path_type& path () const; -- cgit v1.1