Expose rpath entries as a build variable

Unlike commit e2239cc61ca170ff9a65e1a94c3344d2b272c9e2, this only adds "." to the rpath if isNativeDeps is true.

--
PiperOrigin-RevId: 148330144
MOS_MIGRATED_REVID=148330144

1 files changed, 126 insertions, 116 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index 8fd5728a56..0703c604d2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -21,6 +21,7 @@ import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionOwner;
@@ -63,17 +64,9 @@ import javax.annotation.Nullable;
 /** Builder class to construct {@link CppLinkAction}s. */
 public class CppLinkActionBuilder {
 
-  /** A build variable for clang flags that set the root of the linker search path. */
-  public static final String RUNTIME_ROOT_FLAGS_VARIABLE = "runtime_root_flags";
-
-  /** A build variable for entries in the linker search path. */
-  public static final String RUNTIME_ROOT_ENTRIES_VARIABLE = "runtime_root_entries";
-
-  /**
-   * A build variable for options applying to specific libraries in the linker invocation that
-   * either identify a library to be linked or add a directory to the runtime library search path.
-   */
-  public static final String LIBOPTS_VARIABLE = "libopts";
+  /** A build variable for entries in the linker runtime search path (usually set by -rpath flag) */
+  public static final String RUNTIME_LIBRARY_SEARCH_DIRECTORIES_VARIABLE =
+      "runtime_library_search_directories";
 
   public static final String LIBRARY_SEARCH_DIRECTORIES_VARIABLE = "library_search_directories";
 
@@ -134,6 +127,12 @@ public class CppLinkActionBuilder {
   /** A build variable whose presence indicates that this action is a cc_test linking action. */
   public static final String IS_CC_TEST_LINK_ACTION_VARIABLE = "is_cc_test_link_action";
 
+  /**
+   * A (temporary) build variable whose presence indicates that this action is not a cc_test linking
+   * action.
+   */
+  public static final String IS_NOT_CC_TEST_LINK_ACTION_VARIABLE = "is_not_cc_test_link_action";
+
   // Builder-only
   // Null when invoked from tests (e.g. via createTestBuilder).
   @Nullable private final RuleContext ruleContext;
@@ -1217,22 +1216,13 @@ public class CppLinkActionBuilder {
   }
 
   private static class LinkArgCollector {
-    String rpathRoot;
-    ImmutableList<String> rpathEntries;
-    ImmutableSet<String> libopts;
+    ImmutableSet<String> runtimeLibrarySearchDirectories;
     ImmutableSet<String> librarySearchDirectories;
     SequenceBuilder librariesToLink;
 
-    public void setRpathRoot(String rPathRoot) {
-      this.rpathRoot = rPathRoot;
-    }
-
-    public void setRpathEntries(ImmutableList<String> rpathEntries) {
-      this.rpathEntries = rpathEntries;
-    }
-
-    public void setLibopts(ImmutableSet<String> libopts) {
-      this.libopts = libopts;
+    public void setRuntimeLibrarySearchDirectories(
+        ImmutableSet<String> runtimeLibrarySearchDirectories) {
+      this.runtimeLibrarySearchDirectories = runtimeLibrarySearchDirectories;
     }
 
     public void setLibrariesToLink(SequenceBuilder librariesToLink) {
@@ -1243,16 +1233,8 @@ public class CppLinkActionBuilder {
       this.librarySearchDirectories = librarySearchDirectories;
     }
 
-    public String getRpathRoot() {
-      return rpathRoot;
-    }
-
-    public ImmutableList<String> getRpathEntries() {
-      return rpathEntries;
-    }
-
-    public ImmutableSet<String> getLibopts() {
-      return libopts;
+    public ImmutableSet<String> getRuntimeLibrarySearchDirectories() {
+      return runtimeLibrarySearchDirectories;
     }
 
     public SequenceBuilder getLibrariesToLink() {
@@ -1334,21 +1316,16 @@ public class CppLinkActionBuilder {
 
       if (useTestOnlyFlags()) {
         buildVariables.addStringVariable(IS_CC_TEST_LINK_ACTION_VARIABLE, "");
+      } else {
+        buildVariables.addStringVariable(IS_NOT_CC_TEST_LINK_ACTION_VARIABLE, "");
       }
 
-      // rpath
-      if (linkArgCollector.getRpathRoot() != null) {
-        buildVariables.addStringVariable(
-            RUNTIME_ROOT_FLAGS_VARIABLE, linkArgCollector.getRpathRoot());
-      }
-
-      if (linkArgCollector.getRpathEntries() != null) {
+      if (linkArgCollector.getRuntimeLibrarySearchDirectories() != null) {
         buildVariables.addStringSequenceVariable(
-            RUNTIME_ROOT_ENTRIES_VARIABLE, linkArgCollector.getRpathEntries());
+            RUNTIME_LIBRARY_SEARCH_DIRECTORIES_VARIABLE,
+            linkArgCollector.getRuntimeLibrarySearchDirectories());
       }
 
-      buildVariables.addStringSequenceVariable(LIBOPTS_VARIABLE, linkArgCollector.getLibopts());
-
       buildVariables.addCustomBuiltVariable(
           LIBRARIES_TO_LINK_VARIABLE, linkArgCollector.getLibrariesToLink());
 
@@ -1436,10 +1413,9 @@ public class CppLinkActionBuilder {
      * <p>TODO: Factor out of the bazel binary into build variables for crosstool action_configs.
      */
     private void addInputFileLinkOptions(LinkArgCollector linkArgCollector) {
-
-      // Used to collect -L and -Wl,-rpath options, ensuring that each used only once.
-      ImmutableSet.Builder<String> libOpts = ImmutableSet.builder();
       ImmutableSet.Builder<String> librarySearchDirectories = ImmutableSet.builder();
+      ImmutableSet.Builder<String> runtimeRpathRoots = ImmutableSet.builder();
+      ImmutableSet.Builder<String> rpathRootsForExplicitSoDeps = ImmutableSet.builder();
 
       // List of command line parameters that need to be placed *outside* of
       // --whole-archive ... --no-whole-archive.
@@ -1457,14 +1433,11 @@ public class CppLinkActionBuilder {
                   || (linkType == LinkTargetType.EXECUTABLE
                       && linkStaticness == LinkStaticness.DYNAMIC));
 
-      ImmutableList.Builder<String> runtimeRpathEntries = ImmutableList.builder();
-
-      String origin =
-          useTestOnlyFlags && cppConfiguration.supportsExecOrigin()
-              ? "$EXEC_ORIGIN/"
-              : "$ORIGIN/";
       if (runtimeRpath) {
-        runtimeRpathEntries.add("-Wl,-rpath," + origin + runtimeSolibName + "/");
+        if (isNativeDeps) {
+          runtimeRpathRoots.add(".");
+        }
+        runtimeRpathRoots.add(runtimeSolibName + "/");
       }
 
       String rpathRoot;
@@ -1482,10 +1455,9 @@ public class CppLinkActionBuilder {
         // and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared
         // artifact, both are symlinks to the same place, so
         // there's no *one* RPATH setting that fits all targets involved in the sharing.
-        rpathRoot =
-            "-Wl,-rpath," + origin + ":" + origin + cppConfiguration.getSolibDirectory() + "/";
+        rpathRoot = cppConfiguration.getSolibDirectory() + "/";
         if (runtimeRpath) {
-          runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/");
+          runtimeRpathRoots.add("../" + runtimeSolibName + "/");
         }
       } else {
         // For all other links, calculate the relative path from the output file to _solib_[arch]
@@ -1493,18 +1465,14 @@ public class CppLinkActionBuilder {
         // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be
         // "../../_solib_[arch]".
         if (runtimeRpath) {
-          runtimeRpathEntries.add(
-              "-Wl,-rpath,"
-                  + origin
-                  + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1)
+          runtimeRpathRoots.add(
+              Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1)
                   + runtimeSolibName
                   + "/");
         }
 
         rpathRoot =
-            "-Wl,-rpath,"
-                + origin
-                + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1)
+            Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1)
                 + cppConfiguration.getSolibDirectory()
                 + "/";
 
@@ -1512,89 +1480,130 @@ public class CppLinkActionBuilder {
           // We also retain the $ORIGIN/ path to solibs that are in _solib_<arch>, as opposed to
           // the package directory)
           if (runtimeRpath) {
-            runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/");
+            runtimeRpathRoots.add("../" + runtimeSolibName + "/");
           }
-          rpathRoot += ":" + origin;
         }
       }
 
-      boolean includeSolibDir = false;
+      Map<Artifact, Artifact> ltoMap = generateLtoMap();
+      boolean includeSolibDir =
+          addLinkerInputs(
+              librarySearchDirectories,
+              rpathRootsForExplicitSoDeps,
+              librariesToLink,
+              solibDir,
+              rpathRoot,
+              ltoMap);
+      boolean includeRuntimeSolibDir =
+          addRuntimeLinkerInputs(
+              librarySearchDirectories,
+              rpathRootsForExplicitSoDeps,
+              librariesToLink,
+              solibDir,
+              rpathRoot,
+              ltoMap);
+      Preconditions.checkState(
+          ltoMap == null || ltoMap.isEmpty(), "Still have LTO objects left: %s", ltoMap);
 
-      Map<Artifact, Artifact> ltoMap = null;
-      if (!isLTOIndexing && (allLTOArtifacts != null)) {
-        // TODO(bazel-team): The LTO final link can only work if there are individual .o files on
-        // the command line. Rather than crashing, this should issue a nice error. We will get
-        // this by
-        // 1) moving supports_start_end_lib to a toolchain feature
-        // 2) having thin_lto require start_end_lib
-        // As a bonus, we can rephrase --nostart_end_lib as --features=-start_end_lib and get rid
-        // of a command line option.
-
-        Preconditions.checkState(cppConfiguration.useStartEndLib());
-        ltoMap = new HashMap<>();
-        for (LTOBackendArtifacts l : allLTOArtifacts) {
-          ltoMap.put(l.getBitcodeFile(), l.getObjectFile());
-        }
+      ImmutableSet.Builder<String> runtimeLibrarySearchDirectories = ImmutableSet.builder();
+      // rpath ordering matters for performance; first add the one where most libraries are found.
+      if (includeSolibDir) {
+        runtimeLibrarySearchDirectories.add(rpathRoot);
+      }
+      runtimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build());
+      if (includeRuntimeSolibDir) {
+        runtimeLibrarySearchDirectories.addAll(runtimeRpathRoots.build());
       }
 
-      for (LinkerInput input : linkerInputs) {
+      linkArgCollector.setLibrarySearchDirectories(librarySearchDirectories.build());
+      linkArgCollector.setRuntimeLibrarySearchDirectories(runtimeLibrarySearchDirectories.build());
+      linkArgCollector.setLibrariesToLink(librariesToLink);
+    }
+
+    private Map<Artifact, Artifact> generateLtoMap() {
+      if (isLTOIndexing || allLTOArtifacts == null) {
+        return null;
+      }
+      // TODO(bazel-team): The LTO final link can only work if there are individual .o files on
+      // the command line. Rather than crashing, this should issue a nice error. We will get
+      // this by
+      // 1) moving supports_start_end_lib to a toolchain feature
+      // 2) having thin_lto require start_end_lib
+      // As a bonus, we can rephrase --nostart_end_lib as --features=-start_end_lib and get rid
+      // of a command line option.
+
+      Preconditions.checkState(cppConfiguration.useStartEndLib());
+      Map<Artifact, Artifact> ltoMap = new HashMap<>();
+      for (LTOBackendArtifacts l : allLTOArtifacts) {
+        ltoMap.put(l.getBitcodeFile(), l.getObjectFile());
+      }
+      return ltoMap;
+    }
+
+    private boolean addRuntimeLinkerInputs(
+        Builder<String> librarySearchDirectories,
+        Builder<String> rpathRootsForExplicitSoDeps,
+        SequenceBuilder librariesToLink,
+        PathFragment solibDir,
+        String rpathRoot,
+        Map<Artifact, Artifact> ltoMap) {
+      boolean includeRuntimeSolibDir = false;
+      for (LinkerInput input : runtimeLinkerInputs) {
         if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY) {
           PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory();
           Preconditions.checkState(
-              libDir.startsWith(solibDir),
+              runtimeSolibDir != null && libDir.equals(runtimeSolibDir),
               "Artifact '%s' is not under directory '%s'.",
               input.getArtifact(),
               solibDir);
-          if (libDir.equals(solibDir)) {
-            includeSolibDir = true;
-          }
+          includeRuntimeSolibDir = true;
           addDynamicInputLinkOptions(
               input,
               librariesToLink,
-              false,
-              libOpts,
+              true,
               librarySearchDirectories,
+              rpathRootsForExplicitSoDeps,
               solibDir,
               rpathRoot);
         } else {
-          addStaticInputLinkOptions(input, librariesToLink, false, ltoMap);
+          addStaticInputLinkOptions(input, librariesToLink, true, ltoMap);
         }
       }
+      return includeRuntimeSolibDir;
+    }
 
-      boolean includeRuntimeSolibDir = false;
-
-      for (LinkerInput input : runtimeLinkerInputs) {
+    private boolean addLinkerInputs(
+        Builder<String> librarySearchDirectories,
+        Builder<String> rpathEntries,
+        SequenceBuilder librariesToLink,
+        PathFragment solibDir,
+        String rpathRoot,
+        Map<Artifact, Artifact> ltoMap) {
+      boolean includeSolibDir = false;
+      for (LinkerInput input : linkerInputs) {
         if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY) {
           PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory();
           Preconditions.checkState(
-              runtimeSolibDir != null && libDir.equals(runtimeSolibDir),
+              libDir.startsWith(solibDir),
               "Artifact '%s' is not under directory '%s'.",
               input.getArtifact(),
               solibDir);
-          includeRuntimeSolibDir = true;
+          if (libDir.equals(solibDir)) {
+            includeSolibDir = true;
+          }
           addDynamicInputLinkOptions(
-              input, librariesToLink, true, libOpts, librarySearchDirectories, solibDir, rpathRoot);
+              input,
+              librariesToLink,
+              false,
+              librarySearchDirectories,
+              rpathEntries,
+              solibDir,
+              rpathRoot);
         } else {
-          addStaticInputLinkOptions(input, librariesToLink, true, ltoMap);
+          addStaticInputLinkOptions(input, librariesToLink, false, ltoMap);
         }
       }
-
-      // rpath ordering matters for performance; first add the one where most libraries are found.
-      if (includeSolibDir) {
-        linkArgCollector.setRpathRoot(rpathRoot);
-      }
-      if (includeRuntimeSolibDir) {
-        linkArgCollector.setRpathEntries(runtimeRpathEntries.build());
-      }
-
-      linkArgCollector.setLibopts(libOpts.build());
-      linkArgCollector.setLibrarySearchDirectories(librarySearchDirectories.build());
-
-      linkArgCollector.setLibrariesToLink(librariesToLink);
-
-      if (ltoMap != null) {
-        Preconditions.checkState(ltoMap.isEmpty(), "Still have LTO objects left: %s", ltoMap);
-      }
+      return includeSolibDir;
     }
 
     /**
@@ -1606,8 +1615,8 @@ public class CppLinkActionBuilder {
         LinkerInput input,
         SequenceBuilder librariesToLink,
         boolean isRuntimeLinkerInput,
-        ImmutableSet.Builder<String> libOpts,
         ImmutableSet.Builder<String> librarySearchDirectories,
+        ImmutableSet.Builder<String> rpathRootsForExplicitSoDeps,
         PathFragment solibDir,
         String rpathRoot) {
       Preconditions.checkState(input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY);
@@ -1624,7 +1633,8 @@ public class CppLinkActionBuilder {
           commonParent = commonParent.getParentDirectory();
         }
 
-        libOpts.add(rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString());
+        rpathRootsForExplicitSoDeps.add(
+            rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString());
       }
 
       librarySearchDirectories.add(