Rollback of commit 6af85020b520a9dd2bd913562b16716c29c3dbc3.

*** Reason for rollback ***

Breaks ios_test targets.

*** Original change description ***

Add two binary size optimizations when --compilation_mode=opt is specified:
1. Symbol strippings. A new strip action is registered that uses Darwin tool /usr/bin/strip to remove the symbol table of the linked binary.
2. Dead-code strippings, which uses linker flag "--dead_strip" to remove unreachable code in binary link action.

RELNOTES: Perform symbol and dead code strippings on linked binaries generated by ObjC rules.

--
MOS_MIGRATED_REVID=96551473

1 files changed, 24 insertions, 115 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
index 1f346ed7a7..9aaffb63b2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -33,7 +33,6 @@ import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.CLANG_PLU
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.DSYMUTIL;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.NON_ARC_SRCS_TYPE;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.SRCS_TYPE;
-import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.STRIP;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.SWIFT;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -52,9 +51,7 @@ import com.google.devtools.build.lib.analysis.actions.CommandLine;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
-import com.google.devtools.build.lib.analysis.config.CompilationMode;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs;
 import com.google.devtools.build.lib.rules.java.J2ObjcConfiguration;
 import com.google.devtools.build.lib.rules.objc.ObjcCommon.CompilationAttributes;
@@ -381,15 +378,8 @@ final class CompilationSupport {
   }
 
   /**
-   * Registers any actions necessary to link this rule and its dependencies.
-   *
-   * <p>Dsym bundle and breakpad files are generated if
-   * {@link ObjcConfiguration#generateDebugSymbols()} is set.
-   *
-   * <p>When Bazel flag {@code --compilation_mode=opt} is specified, additional optimizations
-   * will be performed on the linked binary: all-symbol stripping (using {@code /usr/bin/strip})
-   * and dead-code stripping (using linker flags: {@code -dead_strip} and
-   * {@code -no_dead_strip_inits_and_terms}).
+   * Registers any actions necessary to link this rule and its dependencies. Debug symbols are
+   * generated if {@link ObjcConfiguration#generateDebugSymbols()} is set.
    *
    * @param objcProvider common information about this rule's attributes and its dependencies
    * @param extraLinkArgs any additional arguments to pass to the linker
@@ -415,19 +405,8 @@ final class CompilationSupport {
 
   private void registerLinkAction(ObjcProvider objcProvider, ExtraLinkArgs extraLinkArgs,
       Iterable<Artifact> extraLinkInputs, Optional<Artifact> dsymBundle) {
-    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
-    IntermediateArtifacts intermediateArtifacts =
-        ObjcRuleClasses.intermediateArtifacts(ruleContext);
-    CompilationMode compilationMode = objcConfiguration.getCompilationMode();
-
-    // When compilation_mode=opt, the unstripped binary containing debug symbols is generated by the
-    // linker, which also needs the debug symbols for dead-code removal. The binary is also used to
-    // generate dSYM bundle if --objc_generate_debug_symbol is specified. A symbol strip action is
-    // later registered to strip the symbol table from the unstripped binary.
-    Artifact binaryToLink =
-        compilationMode == CompilationMode.OPT
-            ? intermediateArtifacts.unstrippedSingleArchitectureBinary()
-            : intermediateArtifacts.strippedSingleArchitectureBinary();
+    Artifact linkedBinary =
+        ObjcRuleClasses.intermediateArtifacts(ruleContext).singleArchitectureBinary();
 
     ImmutableList<Artifact> ccLibraries = ccLibraries(objcProvider);
     ruleContext.registerAction(
@@ -435,8 +414,8 @@ final class CompilationSupport {
             .setMnemonic("ObjcLink")
             .setShellCommand(ImmutableList.of("/bin/bash", "-c"))
             .setCommandLine(
-                linkCommandLine(extraLinkArgs, objcProvider, binaryToLink, dsymBundle, ccLibraries))
-            .addOutput(binaryToLink)
+                linkCommandLine(extraLinkArgs, objcProvider, linkedBinary, dsymBundle, ccLibraries))
+            .addOutput(linkedBinary)
             .addOutputs(dsymBundle.asSet())
             .addTransitiveInputs(objcProvider.get(LIBRARY))
             .addTransitiveInputs(objcProvider.get(IMPORTED_LIBRARY))
@@ -444,24 +423,6 @@ final class CompilationSupport {
             .addInputs(ccLibraries)
             .addInputs(extraLinkInputs)
             .build(ruleContext));
-
-    if (compilationMode == CompilationMode.OPT) {
-      // For test targets, only debug symbols are stripped off, since /usr/bin/strip is not able
-      // to strip off all symbols in XCTest bundle.
-      boolean isTestTarget = TargetUtils.isTestRule(ruleContext.getRule());
-      Iterable<String> stripArgs =
-          isTestTarget ? ImmutableList.of("-S") : ImmutableList.<String>of();
-      Artifact strippedBinary = intermediateArtifacts.strippedSingleArchitectureBinary();
-
-      ruleContext.registerAction(
-          ObjcRuleClasses.spawnOnDarwinActionBuilder()
-              .setMnemonic("ObjcBinarySymbolStrip")
-              .setExecutable(STRIP)
-              .setCommandLine(symbolStripCommandLine(stripArgs, binaryToLink, strippedBinary))
-              .addOutput(strippedBinary)
-              .addInput(binaryToLink)
-              .build(ruleContext));
-    }
   }
 
   private ImmutableList<Artifact> ccLibraries(ObjcProvider objcProvider) {
@@ -472,15 +433,6 @@ final class CompilationSupport {
     return ccLibraryBuilder.build();
   }
 
-  private static CommandLine symbolStripCommandLine(
-      Iterable<String> extraFlags, Artifact unstrippedArtifact, Artifact strippedArtifact) {
-    return CustomCommandLine.builder()
-        .add(extraFlags)
-        .addExecPath("-o", strippedArtifact)
-        .addPath(unstrippedArtifact.getExecPath())
-        .build();
-  }
-
   private CommandLine linkCommandLine(ExtraLinkArgs extraLinkArgs,
       ObjcProvider objcProvider, Artifact linkedBinary, Optional<Artifact> dsymBundle,
       ImmutableList<Artifact> ccLibraries) {
@@ -495,20 +447,10 @@ final class CompilationSupport {
     } else {
       commandLine.addPath(CLANG);
     }
-
-    // Do not perform code stripping on tests because XCTest binary is linked not as an executable
-    // but as a bundle without any entry point.
-    boolean isTestTarget = TargetUtils.isTestRule(ruleContext.getRule());
-    if (objcConfiguration.getCompilationMode() == CompilationMode.OPT && !isTestTarget) {
-      commandLine.add("-dead_strip").add("-no_dead_strip_inits_and_terms");
-    }
-
     commandLine
         .add(IosSdkCommands.commonLinkAndCompileFlagsForClang(objcProvider, objcConfiguration))
-        .add("-Xlinker")
-        .add("-objc_abi_version")
-        .add("-Xlinker")
-        .add("2")
+        .add("-Xlinker").add("-objc_abi_version")
+        .add("-Xlinker").add("2")
         .add("-fobjc-link-runtime")
         .add(IosSdkCommands.DEFAULT_LINKER_FLAGS)
         .addBeforeEach("-framework", frameworkNames(objcProvider))
@@ -715,50 +657,22 @@ final class CompilationSupport {
   private CompilationSupport registerDsymActions() {
     IntermediateArtifacts intermediateArtifacts =
         ObjcRuleClasses.intermediateArtifacts(ruleContext);
-    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
-    CompilationMode compilationMode = objcConfiguration.getCompilationMode();
-
     Artifact dsymBundle = intermediateArtifacts.dsymBundle();
-    Artifact linkedBinary =
-        compilationMode == CompilationMode.OPT
-            ? intermediateArtifacts.unstrippedSingleArchitectureBinary()
-            : intermediateArtifacts.strippedSingleArchitectureBinary();
     Artifact debugSymbolFile = intermediateArtifacts.dsymSymbol();
-    Artifact dsymPlist = intermediateArtifacts.dsymPlist();
-
-    PathFragment dsymOutputDir =
-        replaceSuffix(
-            dsymBundle.getExecPath(), IntermediateArtifacts.TMP_DSYM_BUNDLE_SUFFIX, ".app.dSYM");
-    PathFragment dsymPlistZipEntry = dsymPlist.getExecPath().relativeTo(dsymOutputDir);
-    PathFragment debugSymbolFileZipEntry =
-        debugSymbolFile
-            .getExecPath()
-            .replaceName(linkedBinary.getFilename())
-            .relativeTo(dsymOutputDir);
-
-    StringBuilder unzipDsymCommand = new StringBuilder();
-    unzipDsymCommand
-        .append(
-            String.format(
-                "unzip -p %s %s > %s",
-                dsymBundle.getExecPathString(),
-                dsymPlistZipEntry,
-                dsymPlist.getExecPathString()))
-        .append(
-            String.format(
-                " && unzip -p %s %s > %s",
-                dsymBundle.getExecPathString(),
-                debugSymbolFileZipEntry,
-                debugSymbolFile.getExecPathString()));
-
-    ruleContext.registerAction(
-        new SpawnAction.Builder()
-            .setMnemonic("UnzipDsym")
-            .setShellCommand(unzipDsymCommand.toString())
-            .addInput(dsymBundle)
-            .addOutput(dsymPlist)
-            .addOutput(debugSymbolFile)
-            .build(ruleContext));
+    ruleContext.registerAction(new SpawnAction.Builder()
+        .setMnemonic("UnzipDsym")
+        .setProgressMessage("Unzipping dSYM file: " + ruleContext.getLabel())
+        .setExecutable(new PathFragment("/usr/bin/unzip"))
+        .addInput(dsymBundle)
+        .setCommandLine(CustomCommandLine.builder()
+            .add(dsymBundle.getExecPathString())
+            .add("-d")
+            .add(stripSuffix(dsymBundle.getExecPathString(),
+                IntermediateArtifacts.TMP_DSYM_BUNDLE_SUFFIX) + ".app.dSYM")
+            .build())
+        .addOutput(intermediateArtifacts.dsymPlist())
+        .addOutput(debugSymbolFile)
+        .build(ruleContext));
 
     Artifact dumpsyms = ruleContext.getPrerequisiteArtifact(":dumpsyms", Mode.HOST);
     Artifact breakpadFile = intermediateArtifacts.breakpadSym();
@@ -777,14 +691,9 @@ final class CompilationSupport {
     return this;
   }
 
-  private PathFragment replaceSuffix(PathFragment path, String suffix, String newSuffix) {
+  private String stripSuffix(String str, String suffix) {
     // TODO(bazel-team): Throw instead of returning null?
-    String name = path.getBaseName();
-    if (name.endsWith(suffix)) {
-      return path.replaceName(name.substring(0, name.length() - suffix.length()) + newSuffix);
-    } else {
-      return null;
-    }
+    return str.endsWith(suffix) ? str.substring(0, str.length() - suffix.length()) : null;
   }
 
   /**