Fix C/C++ link rule matching ambiguity by seeing-through utility libraries

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<libx> ())
-        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<file> ();
     }
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<libx> ())
-            pt = &link_member (*l, a, li);
+            pt = link_member (*l, a, li);
 
           bool la;
           if (!((la = pt->is_a<liba> ())  ||
@@ -435,7 +435,7 @@ namespace build2
         if (const target* pt = p.load ())
         {
           if (const libx* l = pt->is_a<libx> ())
-            pt = &link_member (*l, a, li);
+            pt = link_member (*l, a, li);
 
           bool la;
           if (!((la = pt->is_a<liba> ())  ||
@@ -489,7 +489,7 @@ namespace build2
         if (const target* pt = p.load ())
         {
           if (const libx* l = pt->is_a<libx> ())
-            pt = &link_member (*l, a, li);
+            pt = link_member (*l, a, li);
 
           bool la;
           if (!((la = pt->is_a<liba> ())  ||
@@ -741,7 +741,7 @@ namespace build2
             pt = &p.search (t);
 
             if (const libx* l = pt->is_a<libx> ())
-              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<libx> ())
-            lt = &link_member (*l, a, li);
+            lt = link_member (*l, a, li);
           else if (pt->is_a<liba> () || pt->is_a<libs> () || pt->is_a<libux> ())
             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<libx> ())
-          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<libx> ())
-          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<libs> ()) || (at && pt->is_a<liba> ()))
           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<c> ()                  ||
                  // Header-only C library.
                  (lt.library () && p.is_a<h> ()))
         {
-          seen_c = seen_c || true;
+          r.seen_c = r.seen_c || true;
         }
         else if (p.is_a<obj> () || p.is_a<bmi> ())
         {
-          seen_obj = seen_obj || true;
+          r.seen_obj = r.seen_obj || true;
         }
         else if (p.is_a<obje> () || p.is_a<bmie> ())
         {
+          // 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<obja> () || p.is_a<bmia> ())
         {
           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<objs> () || p.is_a<bmis> ())
         {
           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<libx> ()  ||
-                 p.is_a<liba> ()  ||
-                 p.is_a<libs> ()  ||
+        else if (p.is_a<libu> ()  ||
+                 p.is_a<libul> () ||
                  p.is_a<libux> ())
         {
-          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<libx> ())
+            {
+              // 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<libx> ())
+              {
+                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<libx> ());
+              match_result lr (match (a,
+                                      const_cast<target&> (*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<lib> ()  ||
+                 p.is_a<liba> () ||
+                 p.is_a<libs> ())
+        {
+          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<cc> () && !(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<libul> ()) ||
             (ul = pt->is_a<libu>  ()))
         {
-          pf = &link_member (*ul, a, li).as<file> ();
+          pf = &link_member (*ul, a, li)->as<file> ();
         }
         else if ((pf = pt->is_a<libue> ()) ||
                  (pf = pt->is_a<libus> ()) ||
@@ -536,7 +625,7 @@ namespace build2
           // member.
           //
           if (const libx* l = pt->is_a<libx> ())
-            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<lib> ());
 
         // Make sure group members are resolved.
@@ -107,7 +109,7 @@ namespace build2
           }
         }
 
-        return *(ls ? static_cast<const target*> (l.s) : l.a);
+        return ls ? static_cast<const target*> (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