From d1fbcace59846d55c66e741dbc3d59e20ae3e5cf Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 26 Jun 2019 12:28:05 +0200 Subject: Fix C/C++ link rule matching ambiguity by seeing-through utility libraries --- build2/cc/common.cxx | 2 +- build2/cc/compile-rule.cxx | 10 +-- build2/cc/install-rule.cxx | 4 +- build2/cc/link-rule.cxx | 147 ++++++++++++++++++++++++++++++++++++--------- build2/cc/link-rule.hxx | 12 ++++ build2/cc/utility.cxx | 14 +++-- build2/cc/utility.hxx | 7 ++- build2/prerequisite.hxx | 8 +++ 8 files changed, 159 insertions(+), 45 deletions(-) diff --git a/build2/cc/common.cxx b/build2/cc/common.cxx index b4b0a6d..fa774d7 100644 --- a/build2/cc/common.cxx +++ b/build2/cc/common.cxx @@ -485,7 +485,7 @@ namespace build2 // If this is lib{}/libu*{}, pick appropriate member. // if (const libx* l = xt->is_a ()) - xt = &link_member (*l, a, li); // Pick lib*{e,a,s}{}. + xt = link_member (*l, a, li); // Pick lib*{e,a,s}{}. return xt->as (); } diff --git a/build2/cc/compile-rule.cxx b/build2/cc/compile-rule.cxx index 1ea3800..a1796eb 100644 --- a/build2/cc/compile-rule.cxx +++ b/build2/cc/compile-rule.cxx @@ -384,7 +384,7 @@ namespace build2 if (const target* pt = p.load ()) { if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, li); + pt = link_member (*l, a, li); bool la; if (!((la = pt->is_a ()) || @@ -435,7 +435,7 @@ namespace build2 if (const target* pt = p.load ()) { if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, li); + pt = link_member (*l, a, li); bool la; if (!((la = pt->is_a ()) || @@ -489,7 +489,7 @@ namespace build2 if (const target* pt = p.load ()) { if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, li); + pt = link_member (*l, a, li); bool la; if (!((la = pt->is_a ()) || @@ -741,7 +741,7 @@ namespace build2 pt = &p.search (t); if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, li); + pt = link_member (*l, a, li); } else continue; @@ -4713,7 +4713,7 @@ namespace build2 const target* lt (nullptr); if (const libx* l = pt->is_a ()) - lt = &link_member (*l, a, li); + lt = link_member (*l, a, li); else if (pt->is_a () || pt->is_a () || pt->is_a ()) lt = pt; diff --git a/build2/cc/install-rule.cxx b/build2/cc/install-rule.cxx index 00e8a63..5f8722b 100644 --- a/build2/cc/install-rule.cxx +++ b/build2/cc/install-rule.cxx @@ -57,7 +57,7 @@ namespace build2 // link. For libu*{} we want the "see through" logic. // if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, link_info (t.base_scope (), ot)); + pt = link_member (*l, a, link_info (t.base_scope (), ot)); // Note: not redundant since we are returning a member. // @@ -285,7 +285,7 @@ namespace build2 const target* pt (&search (t, p)); if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, link_info (t.base_scope (), ot)); + pt = link_member (*l, a, link_info (t.base_scope (), ot)); if ((st && pt->is_a ()) || (at && pt->is_a ())) return pt->in (t.weak_scope ()) ? pt : nullptr; diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx index 588f3ae..8146ee4 100644 --- a/build2/cc/link-rule.cxx +++ b/build2/cc/link-rule.cxx @@ -43,15 +43,13 @@ namespace build2 "insufficient space"); } - bool link_rule:: - match (action a, target& t, const string& hint) const + link_rule::match_result link_rule:: + match (action a, target& t, ltype lt, bool linkup) const { - tracer trace (x, "link_rule::match"); + // NOTE: the target may be a group (see utility library logic below). - // NOTE: may be called multiple times and for both inner and outer - // operations (see install rules). + match_result r; - ltype lt (link_type (t)); otype ot (lt.type); // If this is a library, link-up to our group (this is the target group @@ -60,12 +58,14 @@ namespace build2 // If we are called for the outer operation (see install rules), then // use resolve_group() to delegate to inner. // - if (lt.library ()) + if (linkup) { if (a.outer ()) resolve_group (a, t); else if (t.group == nullptr) t.group = &search (t, + // @@ Why are we picking libu{} over libul{}? + // lt.utility ? libu::static_type : lib::static_type, t.dir, t.out, t.name); } @@ -76,8 +76,6 @@ namespace build2 // // Note also that we treat bmi{} as obj{}. @@ MODHDR hbmi{}? // - bool seen_x (false), seen_c (false), seen_obj (false), seen_lib (false); - for (prerequisite_member p: group_prerequisite_members (a, t)) { // If excluded or ad hoc, then don't factor it into our tests. @@ -90,59 +88,150 @@ namespace build2 // Header-only X library (or library with C source and X header). (lt.library () && x_header (p, false /* c_hdr */))) { - seen_x = seen_x || true; + r.seen_x = r.seen_x || true; } else if (p.is_a () || // Header-only C library. (lt.library () && p.is_a ())) { - seen_c = seen_c || true; + r.seen_c = r.seen_c || true; } else if (p.is_a () || p.is_a ()) { - seen_obj = seen_obj || true; + r.seen_obj = r.seen_obj || true; } else if (p.is_a () || p.is_a ()) { + // We can make these "no-match" if/when there is a valid use case. + // if (ot != otype::e) fail << p.type ().name << "{} as prerequisite of " << t; - seen_obj = seen_obj || true; + r.seen_obj = r.seen_obj || true; } else if (p.is_a () || p.is_a ()) { if (ot != otype::a) fail << p.type ().name << "{} as prerequisite of " << t; - seen_obj = seen_obj || true; + r.seen_obj = r.seen_obj || true; } else if (p.is_a () || p.is_a ()) { if (ot != otype::s) fail << p.type ().name << "{} as prerequisite of " << t; - seen_obj = seen_obj || true; + r.seen_obj = r.seen_obj || true; } - else if (p.is_a () || - p.is_a () || - p.is_a () || + else if (p.is_a () || + p.is_a () || p.is_a ()) { - seen_lib = seen_lib || true; + // For a unility library we look at its prerequisites, recursively. + // Since these checks are not exactly light-weight, only do them if + // we haven't already seen any X prerequisites. + // + if (!r.seen_x) + { + // This is a bit iffy: in our model a rule can only search a + // target's prerequisites if it matches. But we don't yet know + // whether we match. However, it seems correct to assume that any + // rule-specific search will always resolve to an existing target + // if there is one. So perhaps it's time to relax this restriction + // a little? + // + const target* pt (p.search_existing ()); + + // It's possible we have no group but have a member so try that. + // + if (pt == nullptr && p.is_a ()) + { + // We know this prerequisite member is a prerequisite since + // otherwise the above search would have returned the member + // target. + // + const target_type& tt (ot == otype::a ? libua::static_type : + ot == otype::s ? libus::static_type : + libue::static_type); + + pt = search_existing (p.prerequisite.key (tt)); + } + + if (pt != nullptr) + { + // If this is a group then try to pick (again, if exists) a + // suitable member. If it doesn't exist, then we will only be + // considering the group's prerequisites. + // + if (const libx* gt = pt->is_a ()) + { + linfo li {ot, lorder::a /* unused */}; + if (const target* pm = link_member (*gt, a, li, true)) + pt = pm; + } + + //@@ TMP: const_cast: try to make target in match() const. + + bool group (pt->is_a ()); + match_result lr (match (a, + const_cast (*pt), + (group + ? ltype {ot , true /* utility */} + : link_type (*pt)), + !group /* linkup */)); + + // Do we need to propagate any other seen_* values? Hm, that + // would in fact match with the "see-through" semantics of + // utility libraries we have in other places. + // + r.seen_x = lr.seen_x; + } + else + r.seen_lib = r.seen_lib || true; // Consider as just a library. + } + } + else if (p.is_a () || + p.is_a () || + p.is_a ()) + { + r.seen_lib = r.seen_lib || true; } - // If this is some other c-common header/source (say C++ in a C rule), - // then we shouldn't try to handle that (it may need to be compiled, - // etc). But we assume everyone can handle a C header. + // Some other c-common header/source (say C++ in a C rule) other than + // a C header (we assume everyone can hanle that). // else if (p.is_a () && !(x_header (p, true /* c_hdr */))) { - l4 ([&]{trace << "non-" << x_lang << " prerequisite " << p - << " for target " << t;}); - return false; + r.seen_cc = true; + break; } } - if (!(seen_x || seen_c || seen_obj || seen_lib)) + return r; + } + + bool link_rule:: + match (action a, target& t, const string& hint) const + { + // NOTE: may be called multiple times and for both inner and outer + // operations (see the install rules). + + tracer trace (x, "link_rule::match"); + + ltype lt (link_type (t)); + match_result r (match (a, t, lt, lt.library ())); + + // If this is some other c-common header/source (say C++ in a C rule), + // then we shouldn't try to handle that (it may need to be compiled, + // etc). + // + if (r.seen_cc) + { + l4 ([&]{trace << "non-" << x_lang << " prerequisite " + << "for target " << t;}); + return false; + } + + if (!(r.seen_x || r.seen_c || r.seen_obj || r.seen_lib)) { l4 ([&]{trace << "no " << x_lang << ", C, or obj/lib prerequisite " << "for target " << t;}); @@ -152,7 +241,7 @@ namespace build2 // We will only chain a C source if there is also an X source or we were // explicitly told to. // - if (seen_c && !seen_x && hint < x) + if (r.seen_c && !r.seen_x && hint < x) { l4 ([&]{trace << "C prerequisite without " << x_lang << " or hint " << "for target " << t;}); @@ -333,7 +422,7 @@ namespace build2 if ((ul = pt->is_a ()) || (ul = pt->is_a ())) { - pf = &link_member (*ul, a, li).as (); + pf = &link_member (*ul, a, li)->as (); } else if ((pf = pt->is_a ()) || (pf = pt->is_a ()) || @@ -536,7 +625,7 @@ namespace build2 // member. // if (const libx* l = pt->is_a ()) - pt = &link_member (*l, a, li); + pt = link_member (*l, a, li); } else { diff --git a/build2/cc/link-rule.hxx b/build2/cc/link-rule.hxx index 1a77aef..2df22cf 100644 --- a/build2/cc/link-rule.hxx +++ b/build2/cc/link-rule.hxx @@ -24,6 +24,18 @@ namespace build2 public: link_rule (data&&); + struct match_result + { + bool seen_x = false; + bool seen_c = false; + bool seen_cc = false; + bool seen_obj = false; + bool seen_lib = false; + }; + + match_result + match (action, target&, ltype, bool) const; + virtual bool match (action, target&, const string&) const override; diff --git a/build2/cc/utility.cxx b/build2/cc/utility.cxx index 39f5c35..2751032 100644 --- a/build2/cc/utility.cxx +++ b/build2/cc/utility.cxx @@ -43,8 +43,8 @@ namespace build2 : v.size () > 1 && v[1] == "shared" ? lorder::a_s : lorder::a; } - const target& - link_member (const bin::libx& x, action a, linfo li) + const target* + link_member (const bin::libx& x, action a, linfo li, bool exist) { bool ul; @@ -72,12 +72,14 @@ namespace build2 // Called by the compile rule during execute. // - return phase == run_phase::match - ? search (x, tt, x.dir, x.out, x.name) - : *search_existing (tt, x.dir, x.out, x.name); + return phase == run_phase::match && !exist + ? &search (x, tt, x.dir, x.out, x.name) + : search_existing (tt, x.dir, x.out, x.name); } else { + assert (!exist); + const lib& l (x.as ()); // Make sure group members are resolved. @@ -107,7 +109,7 @@ namespace build2 } } - return *(ls ? static_cast (l.s) : l.a); + return ls ? static_cast (l.s) : l.a; } } } diff --git a/build2/cc/utility.hxx b/build2/cc/utility.hxx index 82d9f71..bc6c3fb 100644 --- a/build2/cc/utility.hxx +++ b/build2/cc/utility.hxx @@ -60,8 +60,11 @@ namespace build2 // Given the link order return the library member to link. That is, liba{} // or libs{} for lib{} and libue{}, libua{} or libus{} for libu*{}. // - const target& - link_member (const bin::libx&, action, linfo); + // If existing is true, then only return the member target if it exists + // (currently only used and supported for utility libraries). + // + const target* + link_member (const bin::libx&, action, linfo, bool existing = false); } } diff --git a/build2/prerequisite.hxx b/build2/prerequisite.hxx index 551751b..258033d 100644 --- a/build2/prerequisite.hxx +++ b/build2/prerequisite.hxx @@ -131,6 +131,14 @@ namespace build2 return prerequisite_key {proj, {&type, &dir, &out, &name, ext}, &scope}; } + // As above but remap the target type to the specified. + // + prerequisite_key + key (const target_type_type& tt) const + { + return prerequisite_key {proj, {&tt, &dir, &out, &name, ext}, &scope}; + } + // Return true if this prerequisite instance (physically) belongs to the // target's prerequisite list. Note that this test only works if you use // references to the container elements and the container hasn't been -- cgit v1.1