RFR: 8151901: test/tools/pack200/Pack200Test fails on verifying native unpacked JAR

[John Rose]

Good.

Please change the comment:
s/null, no tuple change, force recomputation/null, drop ICs attribute, force CP recomputation/

Reordering BSMs and ICs should work.
The BSMs may need extra ICs, but ICs cannot depend on BSMs.
I wonder why we did the other order first?  I guess because that is what the spec. says.

Oh, there's a problem with the attribute insertion order logic; it's buggy that we unconditionally
insert a BSMs attr. and then delete it later.  (That goes wrong when change<0 and we recompute
from scratch.)

Suggested amended patch enclosed.

— John

On Mar 21, 2016, at 12:49 PM, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:
> 
> Hi John,
> 
> This patch contains fixes for two bugs:
> 8151901: test/tools/pack200/Pack200Test fails on verifying native unpacked JAR
> 8062335: Pack200 Java implementation differs from C version, outputs unused UTF8 for BootstrapMethods Note: The test case exhibits both of the above.
> 
> With this  the classes produced by java and the native implementation are congruent,
> though the JDK image's classes exhibit these issues, as a precaution I have extracted the
> minimal set of classes to reproduce the issues, into the golden jar.
> 
> The webrev is at:
> http://cr.openjdk.java.net/~ksrini/8151901/webrev.00/
> 
> Thanks
> Kumar
> 

-------------- next part --------------

diff --git a/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java b/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
@@ -312,6 +312,12 @@
         void setBootstrapMethods(Collection<BootstrapMethodEntry> bsms) {
             assert(bootstrapMethods == null);  // do not do this twice
             bootstrapMethods = new ArrayList<>(bsms);
+            // Edit the attribute list, if necessary.
+            Attribute a = getAttribute(attrBootstrapMethodsEmpty);
+            if (bootstrapMethods != null && a == null)
+                addAttribute(attrBootstrapMethodsEmpty.canonicalInstance());
+            else if (bootstrapMethods == null && a != null)
+                removeAttribute(a);
         }
 
         boolean hasInnerClasses() {
diff --git a/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java b/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
@@ -1193,16 +1193,25 @@
         cls.visitRefs(VRM_CLASSIC, cpRefs);
 
         ArrayList<BootstrapMethodEntry> bsms = new ArrayList<>();
-        /*
-         * BootstrapMethod(BSMs) are added here before InnerClasses(ICs),
-         * so as to ensure the order. Noting that the BSMs  may be
-         * removed if they are not found in the CP, after the ICs expansion.
-         */
-        cls.addAttribute(Package.attrBootstrapMethodsEmpty.canonicalInstance());
 
         // flesh out the local constant pool
         ConstantPool.completeReferencesIn(cpRefs, true, bsms);
 
+        // Add the BSMs and references as required.
+        if (!bsms.isEmpty()) {
+            // Add the attirbute name to the CP (visitRefs would have wanted this).
+            cpRefs.add(Package.getRefString("BootstrapMethods"));
+            // The pack-spec requires that BootstrapMethods precedes the InnerClasses attribute.
+            // So add BSMs now before running expandLocalICs.
+            // But first delete any local InnerClasses attr.  (This is rare.)
+            List<InnerClass> localICs = cls.getInnerClasses();
+            cls.setInnerClasses(null);
+            Collections.sort(bsms);
+            cls.setBootstrapMethods(bsms);
+            // Re-add the ICs, so the attribute lists are in the required order.
+            cls.setInnerClasses(localICs);
+        }
+
         // Now that we know all our local class references,
         // compute the InnerClasses attribute.
         int changed = cls.expandLocalICs();
@@ -1221,16 +1230,6 @@
             ConstantPool.completeReferencesIn(cpRefs, true, bsms);
         }
 
-        // remove the attr previously set, otherwise add the bsm and
-        // references as required
-        if (bsms.isEmpty()) {
-            cls.attributes.remove(Package.attrBootstrapMethodsEmpty.canonicalInstance());
-        } else {
-            cpRefs.add(Package.getRefString("BootstrapMethods"));
-            Collections.sort(bsms);
-            cls.setBootstrapMethods(bsms);
-        }
-
         // construct a local constant pool
         int numDoubles = 0;
         for (Entry e : cpRefs) {