Use pic mode for cc_fake_binary if pic actions are used for binaries.

To allow the commands in the cc_fake_binary to work with modules (which is
a precondition to writing nocompile tests for modules), we need to create
header modules in the same pic/nopic mode as the rest of the commandline
indicates.

There are two ways to resolve this problem:
a) Make cc_fake_binary use the same pic mode as other binaries.
b) Make sure fake compile actions get all their inputs in non-pic mode.

This patch proposes solution a), as that makes sure cc_fake_binary is as close
to the actual compilation going on as possible; for example, if we decide to
always use pic in the future, this will simply work; additionally, we will
currently get more test coverage through different compilation modes.

RELNOTES: 'cc_fake_binary' uses the same pic/nopic mode as other binaries.

--
MOS_MIGRATED_REVID=99902738

5 files changed, 61 insertions, 33 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 4be3a57e69..815af7005d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -359,7 +359,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
         .addNonLibraryInputs(compilationPrerequisites);
 
     // Determine the object files to link in.
-    boolean usePic = CppHelper.usePic(context, !isLinkShared(context)) && !fake;
+    boolean usePic = CppHelper.usePic(context, !isLinkShared(context));
     Iterable<Artifact> compiledObjectFiles = compilationOutputs.getObjectFiles(usePic);
 
     if (fake) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 41ebc91ac9..a943057030 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -263,7 +263,8 @@ public class CppCompileActionBuilder {
           variables, sourceFile, sourceLabel, realMandatoryInputsBuilder.build(), outputFile,
           tempOutputFile, dotdFile, configuration, cppConfiguration, context, actionContext,
           ImmutableList.copyOf(copts), ImmutableList.copyOf(pluginOpts),
-          getNocoptPredicate(nocopts), extraSystemIncludePrefixes, fdoBuildStamp, ruleContext);
+          getNocoptPredicate(nocopts), extraSystemIncludePrefixes, fdoBuildStamp, ruleContext,
+          usePic);
     } else {
       NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index ceeff8cd12..cc0caa11d4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -266,7 +266,8 @@ public final class CppLinkAction extends AbstractAction {
       // Concatenate all the (fake) .o files into the result.
       for (LinkerInput linkerInput : getLinkCommandLine().getLinkerInputs()) {
         Artifact objectFile = linkerInput.getArtifact();
-        if (CppFileTypes.OBJECT_FILE.matches(objectFile.getFilename())
+        if ((CppFileTypes.OBJECT_FILE.matches(objectFile.getFilename())
+                || CppFileTypes.PIC_OBJECT_FILE.matches(objectFile.getFilename()))
             && linkerInput.isFake()) {
           s.append(FileSystemUtils.readContentAsLatin1(objectFile.getPath())); // (IOException)
         }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 0556819d90..5282dc1188 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -437,31 +437,15 @@ public final class CppModel {
       builder.setContext(CppCompilationContext.mergeForLipo(lipoProvider.getLipoContext(),
           context));
     }
+    boolean generatePicAction = getGeneratePicActions();
+    // If we always need pic for everything, then don't bother to create a no-pic action.
+    boolean generateNoPicAction = getGenerateNoPicActions();
+    Preconditions.checkState(generatePicAction || generateNoPicAction);
     if (fake) {
-      // For cc_fake_binary, we only create a single fake compile action. It's
-      // not necessary to use -fPIC for negative compilation tests, and using
-      // .pic.o files in cc_fake_binary would break existing uses of
-      // cc_fake_binary.
-      Artifact outputFile = ruleContext.getRelatedArtifact(outputName, outputExtension);
-      PathFragment tempOutputName =
-          FileSystemUtils.replaceExtension(outputFile.getExecPath(), ".temp" + outputExtension);
-      builder
-          .setOutputFile(outputFile)
-          .setDotdFile(outputName, dependencyFileExtension)
-          .setTempOutputFile(tempOutputName);
-      setupBuildVariables(builder, getGeneratePicActions(), ccRelativeName);
-      semantics.finalizeCompileActionBuilder(ruleContext, builder);
-      CppCompileAction action = builder.build();
-      env.registerAction(action);
-      if (addObject) {
-        result.addObjectFile(action.getOutputFile());
-      }
+      boolean usePic = !generateNoPicAction;
+      createFakeSourceAction(outputName, result, env, builder, outputExtension,
+          dependencyFileExtension, addObject, ccRelativeName, usePic);
     } else {
-      boolean generatePicAction = getGeneratePicActions();
-      // If we always need pic for everything, then don't bother to create a no-pic action.
-      boolean generateNoPicAction = getGenerateNoPicActions();
-      Preconditions.checkState(generatePicAction || generateNoPicAction);
-
       // Create PIC compile actions (same as non-PIC, but use -fPIC and
       // generate .pic.o, .pic.d, .pic.gcno instead of .o, .d, .gcno.)
       if (generatePicAction) {
@@ -529,6 +513,36 @@ public final class CppModel {
     }
   }
 
+  private void createFakeSourceAction(PathFragment outputName, CcCompilationOutputs.Builder result,
+      AnalysisEnvironment env, CppCompileActionBuilder builder, String outputExtension,
+      String dependencyFileExtension, boolean addObject, PathFragment ccRelativeName,
+      boolean usePic) {
+    if (usePic) {
+      outputExtension = ".pic" + outputExtension;
+      dependencyFileExtension = ".pic" + dependencyFileExtension;
+    }
+    Artifact outputFile = ruleContext.getRelatedArtifact(outputName, outputExtension);
+    PathFragment tempOutputName =
+        FileSystemUtils.replaceExtension(
+            outputFile.getExecPath(), ".temp" + outputExtension, outputExtension);
+    builder
+        .setPicMode(usePic)
+        .setOutputFile(outputFile)
+        .setDotdFile(outputName, dependencyFileExtension)
+        .setTempOutputFile(tempOutputName);
+    setupBuildVariables(builder, getGeneratePicActions(), ccRelativeName);
+    semantics.finalizeCompileActionBuilder(ruleContext, builder);
+    CppCompileAction action = builder.build();
+    env.registerAction(action);
+    if (addObject) {
+      if (usePic) {
+        result.addPicObjectFile(action.getOutputFile());
+      } else {
+        result.addObjectFile(action.getOutputFile());
+      }
+    }
+  }
+
   /**
    * Constructs the C++ linker actions. It generally generates two actions, one for a static library
    * and one for a dynamic library. If PIC is required for shared libraries, but not for binaries,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index e273031a72..f309c689b6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -59,7 +59,8 @@ public class FakeCppCompileAction extends CppCompileAction {
 
   private final PathFragment tempOutputFile;
 
-  FakeCppCompileAction(ActionOwner owner,
+  FakeCppCompileAction(
+      ActionOwner owner,
       ImmutableList<String> features,
       FeatureConfiguration featureConfiguration,
       CcToolchainFeatures.Variables variables,
@@ -78,10 +79,22 @@ public class FakeCppCompileAction extends CppCompileAction {
       Predicate<String> nocopts,
       ImmutableList<PathFragment> extraSystemIncludePrefixes,
       @Nullable String fdoBuildStamp,
-      RuleContext ruleContext) {
-    super(owner, features, featureConfiguration, variables, sourceFile, sourceLabel,
-        mandatoryInputs, outputFile, dotdFile, null, null, null,
-        configuration, cppConfiguration,
+      RuleContext ruleContext,
+      boolean usePic) {
+    super(owner,
+        features,
+        featureConfiguration,
+        variables,
+        sourceFile,
+        sourceLabel,
+        mandatoryInputs,
+        outputFile,
+        dotdFile,
+        null,
+        null,
+        null,
+        configuration,
+        cppConfiguration,
         // We only allow inclusion of header files explicitly declared in
         // "srcs", so we only use declaredIncludeSrcs, not declaredIncludeDirs.
         // (Disallowing use of undeclared headers for cc_fake_binary is needed
@@ -91,8 +104,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         // time, so they can't depend on the contents of the ".d" file.)
         CppCompilationContext.disallowUndeclaredHeaders(context), actionContext, copts, pluginOpts,
         nocopts, extraSystemIncludePrefixes, fdoBuildStamp, VOID_INCLUDE_RESOLVER,
-        ImmutableList.<IncludeScannable>of(),
-        GUID, /*usePic=*/false, ruleContext);
+        ImmutableList.<IncludeScannable>of(), GUID, usePic, ruleContext);
     this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile);
   }