From fb9f2206a3a9b860480d2e9967561b47c1e86351 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 16 Dec 2021 07:44:54 +0200
Subject: Don't consider implied existing targets in dyndep logic

While considering implied targets should be harmless, the result is racy.
---
 libbuild2/dyndep.cxx | 44 ++++++++++++++++++++++++++++++++++++--------
 libbuild2/dyndep.hxx |  4 +++-
 2 files changed, 39 insertions(+), 9 deletions(-)

(limited to 'libbuild2')

diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index 3740e21..ed41e0a 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -523,18 +523,43 @@ namespace build2
       if (!insert || tts.size () > 1)
       {
         // Note that we skip any target type-specific searches (like for an
-        // existing file) and go straight for the target object since we
-        // need to find the target explicitly spelled out.
+        // existing file) and go straight for the target object since we need
+        // to find the target explicitly spelled out.
         //
-        // Also, it doesn't feel like we should be able to resolve an
-        // absolute path with a spelled-out extension to multiple targets.
+        // Also, it doesn't feel like we should be able to resolve an absolute
+        // path with a spelled-out extension to multiple targets.
         //
-        for (const target_type* tt: tts)
+        const target* f (nullptr);
+
+        for (size_t i (0), m (tts.size ()); i != m; ++i)
         {
-          if ((r = t.ctx.targets.find (*tt, d, out, n, e, trace)) != nullptr)
-            break;
+          const target_type& tt (*tts[i]);
+
+          if (const target* x = t.ctx.targets.find (tt, d, out, n, e, trace))
+          {
+            // What would be the harm in reusing an implied target if there is
+            // no real one? Probably none (since it can't be updated) except
+            // that it will be racy: sometimes we will reuse the implied,
+            // sometimes we will insert a new one. And we don't like racy.
+            //
+            if (x->decl == target_decl::real)
+            {
+              r = x;
+              break;
+            }
+            else
+            {
+              // Cache the implied target corresponding to tts[0] since that's
+              // what we will be inserting (see below).
+              //
+              if (insert && i == 0)
+                f = x;
+
+              l6 ([&]{trace << "implied target with target type " << tt.name;});
+            }
+          }
           else
-            l6 ([&]{trace << "no targe with target type " << tt->name;});
+            l6 ([&]{trace << "no target with target type " << tt.name;});
         }
 
         // Note: we can't do this because of the in-source builds where there
@@ -561,6 +586,9 @@ namespace build2
           dr << info << "spell-out its target to resolve this ambiguity";
         }
 #endif
+
+        if (r == nullptr && f != nullptr)
+          r = f;
       }
 
       // @@ OPT: move d, out, n
diff --git a/libbuild2/dyndep.hxx b/libbuild2/dyndep.hxx
index cfd3c7e..ad95df1 100644
--- a/libbuild2/dyndep.hxx
+++ b/libbuild2/dyndep.hxx
@@ -198,7 +198,9 @@ namespace build2
                 const function<prefix_map_func>& = nullptr,
                 const srcout_map& = {});
 
-    // As above but do not insert the target if it doesn't already exist.
+    // As above but do not insert the target if it doesn't already exist. This
+    // function also returns NULL if the target exists but is implied (that
+    // is, not declared in a buildfile).
     //
     static pair<const file*, bool>
     find_file (tracer&, const char* what,
-- 
cgit v1.1