Prune anonymous classes in ijar

--
MOS_MIGRATED_REVID=119581073

5 files changed, 195 insertions, 82 deletions

diff --git a/third_party/ijar/classfile.cc b/third_party/ijar/classfile.cc
index 04e54221b9..9d48429e0c 100644
--- a/third_party/ijar/classfile.cc
+++ b/third_party/ijar/classfile.cc
@@ -72,17 +72,19 @@ enum CONSTANT {
 };
 
 // See Tables 4.1, 4.4, 4.5 in JVM Spec.
-enum ACCESS  {
-  ACC_PUBLIC          = 0x0001,
-  ACC_PRIVATE         = 0x0002,
-  ACC_PROTECTED       = 0x0004,
-  ACC_STATIC          = 0x0008,
-  ACC_FINAL           = 0x0010,
-  ACC_SYNCHRONIZED    = 0x0020,
-  ACC_VOLATILE        = 0x0040,
-  ACC_TRANSIENT       = 0x0080,
-  ACC_INTERFACE       = 0x0200,
-  ACC_ABSTRACT        = 0x0400
+enum ACCESS {
+  ACC_PUBLIC = 0x0001,
+  ACC_PRIVATE = 0x0002,
+  ACC_PROTECTED = 0x0004,
+  ACC_STATIC = 0x0008,
+  ACC_FINAL = 0x0010,
+  ACC_SYNCHRONIZED = 0x0020,
+  ACC_BRIDGE = 0x0040,
+  ACC_VOLATILE = 0x0040,
+  ACC_TRANSIENT = 0x0080,
+  ACC_INTERFACE = 0x0200,
+  ACC_ABSTRACT = 0x0400,
+  ACC_SYNTHETIC = 0x1000
 };
 
 // See Table 4.7.20-A in Java 8 JVM Spec.
@@ -503,21 +505,22 @@ struct InnerClassesAttribute : Attribute {
            ++i_entry) {
         Entry* entry = entries_[i_entry];
         if (entry->inner_class_info->Kept() ||
-            used_class_names.find(entry->inner_class_info->Display())
-                != used_class_names.end() ||
-            entry->outer_class_info == class_name ||
-            entry->outer_class_info == NULL ||
-            entry->inner_name == NULL) {
+            used_class_names.find(entry->inner_class_info->Display()) !=
+                used_class_names.end() ||
+            entry->outer_class_info == class_name) {
+          if (entry->inner_name == NULL) {
+            // JVMS 4.7.6: inner_name_index is zero iff the class is anonymous
+            continue;
+          }
+
           kept_entries.insert(i_entry);
 
-          // These are zero for anonymous inner classes
+          // JVMS 4.7.6: outer_class_info_index is zero for top-level classes
           if (entry->outer_class_info != NULL) {
             entry->outer_class_info->slot();
           }
 
-          if (entry->inner_name != NULL) {
-            entry->inner_name->slot();
-          }
+          entry->inner_name->slot();
         }
       }
       iteration += 1;
@@ -1292,7 +1295,7 @@ struct ClassFile : HasAttrs {
 
   bool ReadConstantPool(const u1 *&p);
 
-  void StripIfAnonymous();
+  bool IsLocalOrAnonymous();
 
   void WriteHeader(u1 *&p) {
     put_u4be(p, magic);
@@ -1524,53 +1527,15 @@ bool ClassFile::ReadConstantPool(const u1 *&p) {
   return true;
 }
 
-// Anonymous inner classes are stripped to opaque classes that only extend
-// Object. None of their methods or fields are accessible anyway.
-void ClassFile::StripIfAnonymous() {
-  int enclosing_index = -1;
-  int inner_classes_index = -1;
-
-  for (size_t ii = 0; ii < attributes.size(); ++ii) {
-    if (attributes[ii]->attribute_name_->Display() == "EnclosingMethod") {
-      enclosing_index = ii;
-    } else if (attributes[ii]->attribute_name_->Display() == "InnerClasses") {
-      inner_classes_index = ii;
-    }
-  }
-
-  // Presence of an EnclosingMethod attribute indicates a local or anonymous
-  // class, which can be stripped.
-  if (enclosing_index > -1) {
-    // Clear the signature to only extend java.lang.Object.
-    super_class = NULL;
-    interfaces.clear();
-
-    // Clear away all fields (implementation details).
-    for (size_t ii = 0; ii < fields.size(); ++ii) {
-      delete fields[ii];
-    }
-    fields.clear();
-
-    // Clear away all methods (implementation details).
-    for (size_t ii = 0; ii < methods.size(); ++ii) {
-      delete methods[ii];
-    }
-    methods.clear();
-
-    // Only preserve the InnerClasses attribute to comply with the spec.
-    Attribute *attr = NULL;
-    for (size_t ii = 0; ii < attributes.size(); ++ii) {
-      if (static_cast<int>(ii) != inner_classes_index) {
-        delete attributes[ii];
-      } else {
-        attr = attributes[ii];
-      }
-    }
-    attributes.clear();
-    if (attr != NULL) {
-      attributes.push_back(attr);
+bool ClassFile::IsLocalOrAnonymous() {
+  for (const Attribute *attribute : attributes) {
+    if (attribute->attribute_name_->Display() == "EnclosingMethod") {
+      // JVMS 4.7.6: a class must has EnclosingMethod attribute iff it
+      // represents a local class or an anonymous class
+      return true;
     }
   }
+  return false;
 }
 
 static ClassFile *ReadClass(const void *classdata, size_t length) {
@@ -1609,9 +1574,11 @@ static ClassFile *ReadClass(const void *classdata, size_t length) {
   for (int ii = 0; ii < fields_count; ++ii) {
     Member *field = Member::Read(p);
 
-    if (!(field->access_flags & ACC_PRIVATE)) { // drop private fields
-      clazz->fields.push_back(field);
+    if ((field->access_flags & ACC_PRIVATE) == ACC_PRIVATE) {
+      // drop private fields
+      continue;
     }
+    clazz->fields.push_back(field);
   }
 
   u2 methods_count = get_u2be(p);
@@ -1621,13 +1588,21 @@ static ClassFile *ReadClass(const void *classdata, size_t length) {
     // drop class initializers
     if (method->name->Display() == "<clinit>") continue;
 
-    if (!(method->access_flags & ACC_PRIVATE)) { // drop private methods
-      clazz->methods.push_back(method);
+    if ((method->access_flags & ACC_PRIVATE) == ACC_PRIVATE) {
+      // drop private methods
+      continue;
     }
+    if ((method->access_flags & (ACC_SYNTHETIC | ACC_BRIDGE)) ==
+        ACC_SYNTHETIC) {
+      // drop non-bridge synthetic methods, e.g. package-private synthetic
+      // constructors used to instantiate private nested classes within their
+      // declaring compilation unit
+      continue;
+    }
+    clazz->methods.push_back(method);
   }
 
   clazz->ReadAttrs(p);
-  clazz->StripIfAnonymous();
 
   return clazz;
 }
@@ -1807,12 +1782,14 @@ void ClassFile::WriteClass(u1 *&p) {
   delete[] body;
 }
 
-
-void StripClass(u1 *&classdata_out, const u1 *classdata_in, size_t in_length) {
+bool StripClass(u1 *&classdata_out, const u1 *classdata_in, size_t in_length) {
   ClassFile *clazz = ReadClass(classdata_in, in_length);
+  bool keep = true;
   if (clazz == NULL) {
     // Class is invalid. Simply copy it to the output and call it a day.
     put_n(classdata_out, classdata_in, in_length);
+  } else if (clazz->IsLocalOrAnonymous()) {
+    keep = false;
   } else {
 
     // Constant pool item zero is a dummy entry.  Setting it marks the
@@ -1832,6 +1809,7 @@ void StripClass(u1 *&classdata_out, const u1 *classdata_in, size_t in_length) {
 
   const_pool_in.clear();
   const_pool_out.clear();
+  return keep;
 }
 
 }  // namespace devtools_ijar
diff --git a/third_party/ijar/ijar.cc b/third_party/ijar/ijar.cc
index f6888d4853..1f1c782879 100644
--- a/third_party/ijar/ijar.cc
+++ b/third_party/ijar/ijar.cc
@@ -30,8 +30,8 @@ bool verbose = false;
 
 // Reads a JVM class from classdata_in (of the specified length), and
 // writes out a simplified class to classdata_out, advancing the
-// pointer.
-void StripClass(u1 *&classdata_out, const u1 *classdata_in, size_t in_length);
+// pointer. Returns true if the class should be kept.
+bool StripClass(u1*& classdata_out, const u1* classdata_in, size_t in_length);
 
 const char* CLASS_EXTENSION = ".class";
 const size_t CLASS_EXTENSION_LENGTH = strlen(CLASS_EXTENSION);
@@ -74,11 +74,17 @@ void JarStripperProcessor::Process(const char* filename, const u4 attr,
   if (verbose) {
     fprintf(stderr, "INFO: StripClass: %s\n", filename);
   }
-  u1 *q = builder->NewFile(filename, 0);
-  u1 *classdata_out = q;
-  StripClass(q, data, size);  // actually process it
-  size_t out_length = q - classdata_out;
+  u1* buf = reinterpret_cast<u1*>(malloc(size));
+  u1* classdata_out = buf;
+  if (!StripClass(buf, data, size)) {
+    free(classdata_out);
+    return;
+  }
+  u1* q = builder->NewFile(filename, 0);
+  size_t out_length = buf - classdata_out;
+  memcpy(q, classdata_out, out_length);
   builder->FinishFile(out_length);
+  free(classdata_out);
 }
 
 // Opens "file_in" (a .jar file) for reading, and writes an interface
diff --git a/third_party/ijar/test/BUILD b/third_party/ijar/test/BUILD
index 7dbfb600d1..f8ebb2a612 100644
--- a/third_party/ijar/test/BUILD
+++ b/third_party/ijar/test/BUILD
@@ -128,6 +128,19 @@ genrule(
     tools = ["gen_source_debug_extension"],
 )
 
+java_library(
+    name = "local_and_anonymous_lib",
+    srcs = ["LocalAndAnonymous.java"],
+)
+
+genrule(
+    name = "local_and_anonymous_interface",
+    srcs = [":liblocal_and_anonymous_lib.jar"],
+    outs = ["local_and_anonymous-interface.jar"],
+    cmd = "$(location //third_party/ijar) $< $@",
+    tools = ["//third_party/ijar"],
+)
+
 java_test(
     name = "IjarTests",
     size = "small",
@@ -141,13 +154,17 @@ java_test(
         "UseRestrictedAnnotation.java",
         "package-info.java",
         ":interface_ijar_testlib",
+        ":liblocal_and_anonymous_lib.jar",
+        ":local_and_anonymous-interface.jar",
     ],
     tags = ["zip"],
     test_class = "IjarTests",
     deps = [
         "//src/java_tools/buildjar:BazelJavaCompiler",
         "//third_party:asm",
+        "//third_party:guava",
         "//third_party:junit4",
+        "//third_party:truth",
     ],
 )
 
diff --git a/third_party/ijar/test/IjarTests.java b/third_party/ijar/test/IjarTests.java
index 18e94f2145..7abdbb7868 100644
--- a/third_party/ijar/test/IjarTests.java
+++ b/third_party/ijar/test/IjarTests.java
@@ -12,25 +12,33 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.fail;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.common.io.ByteStreams;
 import com.google.devtools.build.java.bazel.BazelJavaCompiler;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
-
 import org.objectweb.asm.ClassReader;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.Opcodes;
 
 import java.io.File;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
@@ -43,7 +51,6 @@ import javax.tools.JavaCompiler;
 import javax.tools.JavaFileObject;
 import javax.tools.StandardJavaFileManager;
 import javax.tools.StandardLocation;
-import javax.tools.ToolProvider;
 
 /**
  * JUnit tests for ijar tool.
@@ -52,7 +59,7 @@ import javax.tools.ToolProvider;
 public class IjarTests {
 
   private static File getTmpDir() {
-    String tmpdir = System.getenv("TEST_TMPDIR");    
+    String tmpdir = System.getenv("TEST_TMPDIR");
     if (tmpdir == null) {
       // Fall back on the system temporary directory
       tmpdir = System.getProperty("java.io.tmpdir");
@@ -191,4 +198,72 @@ public class IjarTests {
     }
     return builder.toString();
   }
+
+  @Test
+  public void localAndAnonymous() throws Exception {
+    Map<String, byte[]> lib = readJar("third_party/ijar/test/liblocal_and_anonymous_lib.jar");
+    Map<String, byte[]> ijar = readJar("third_party/ijar/test/local_and_anonymous-interface.jar");
+
+    assertThat(lib.keySet())
+        .containsExactly(
+            "LocalAndAnonymous$1.class",
+            "LocalAndAnonymous$2.class",
+            "LocalAndAnonymous$1LocalClass.class",
+            "LocalAndAnonymous.class",
+            "LocalAndAnonymous$NestedClass.class",
+            "LocalAndAnonymous$InnerClass.class",
+            "LocalAndAnonymous$PrivateInnerClass.class");
+    assertThat(ijar.keySet())
+        .containsExactly(
+            "LocalAndAnonymous.class",
+            "LocalAndAnonymous$NestedClass.class",
+            "LocalAndAnonymous$InnerClass.class",
+            "LocalAndAnonymous$PrivateInnerClass.class");
+
+    assertThat(innerClasses(lib.get("LocalAndAnonymous.class")))
+        .isEqualTo(
+            ImmutableMap.<String, String>builder()
+                .put("LocalAndAnonymous$1", "null")
+                .put("LocalAndAnonymous$2", "null")
+                .put("LocalAndAnonymous$1LocalClass", "null")
+                .put("LocalAndAnonymous$InnerClass", "LocalAndAnonymous")
+                .put("LocalAndAnonymous$NestedClass", "LocalAndAnonymous")
+                .put("LocalAndAnonymous$PrivateInnerClass", "LocalAndAnonymous")
+                .build());
+    assertThat(innerClasses(ijar.get("LocalAndAnonymous.class")))
+        .containsExactly(
+            "LocalAndAnonymous$InnerClass", "LocalAndAnonymous",
+            "LocalAndAnonymous$NestedClass", "LocalAndAnonymous",
+            "LocalAndAnonymous$PrivateInnerClass", "LocalAndAnonymous");
+  }
+
+  static Map<String, byte[]> readJar(String path) throws IOException {
+    Map<String, byte[]> classes = new HashMap<>();
+    try (JarFile jf = new JarFile(path)) {
+      Enumeration<JarEntry> entries = jf.entries();
+      while (entries.hasMoreElements()) {
+        JarEntry je = entries.nextElement();
+        if (!je.getName().endsWith(".class")) {
+          continue;
+        }
+        classes.put(je.getName(), ByteStreams.toByteArray(jf.getInputStream(je)));
+      }
+    }
+    return classes;
+  }
+
+  static Map<String, String> innerClasses(byte[] bytes) {
+    final Map<String, String> innerClasses = new HashMap<>();
+    new ClassReader(bytes)
+        .accept(
+            new ClassVisitor(Opcodes.ASM5) {
+              @Override
+              public void visitInnerClass(
+                  String name, String outerName, String innerName, int access) {
+                innerClasses.put(name, String.valueOf(outerName));
+              }
+            },
+            /*flags=*/ 0);
+    return innerClasses;
+  }
 }
diff --git a/third_party/ijar/test/LocalAndAnonymous.java b/third_party/ijar/test/LocalAndAnonymous.java
new file mode 100644
index 0000000000..00c16d863f
--- /dev/null
+++ b/third_party/ijar/test/LocalAndAnonymous.java
@@ -0,0 +1,37 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * A class containing anonymous and local declarations.
+ */
+public class LocalAndAnonymous {
+
+  public static void main(String[] args) {
+    new LocalAndAnonymous() {}; // anonymous
+    class LocalClass extends LocalAndAnonymous {} // local
+    new LocalClass() {}; // yes
+  }
+
+  void f() {
+    new PrivateInnerClass();
+  }
+
+  static class NestedClass {}
+
+  class InnerClass {}
+
+  private class PrivateInnerClass {
+    private PrivateInnerClass() {}
+  }
+}