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

[Kumar Srinivasan]

Hi John,

Please review the amended patch, based on our discussions.
http://cr.openjdk.java.net/~ksrini/8151901/webrev.01/

Highlights:
* adjust the ordering of the InnerClass and BootStrapMethods computations
* enabled the test, and disabled memory leak check, which causes 
intermittent
   failures, because of GC's ergonomics etc.
* added the two test files to golden.jar, just in case.

Thanks
Kumar



> 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
>>
>
>
> 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) {
>