Invalid reordering in ClassReader

[Eric McCorkle]

On 02/25/14 11:11, Werner Dietl wrote:
> Ah, there is no test case for this. I'm not sure how to test for this
> - you want to make sure that ClassReader/ClassWriter are consistent.
> This particular re-ordering mistake might get unnoticed if the bytes
> happen to align in both orderings.

The langtools team has a rule that any patch that is intended to
introduce a behavior change must be accompanied by a test.  While this
does slow down development, particularly when we encounter an area where
we don't have very good facilities for writing tests, it also ensures
continually increasing quality.

> Why did the changeset that introduced the bug go in without test case?

It was a noreg-cleanup changeset: meaning it was not supposed to have
any visible effects, as it was supposedly just code cleanup.

The reordering was a simple oversight on my part, but the real issues is
that our test suite allowed it to slip through, because it is deficient
in these cases.  Hence why it's important that we submit a test along
with this patch.

> Could you at least add the issue that this problem doesn't get lost?

I somehow thought that an issue already existed.  I've opened one here:
https://bugs.openjdk.java.net/browse/JDK-8035763

> 
> cu, WMD.
> 
> 
> On Tue, Feb 25, 2014 at 10:54 AM, Eric McCorkle
> <eric.mccorkle at oracle.com> wrote:
>> I've been working on other patches.  The only reason I haven't put this
>> in already is that I'd need to also develop a test for it.  Is there a
>> test for this in the type annotations repo?  If so, I could commit it to
>> the main repository.
>>
>> On 02/25/14 10:38, Werner Dietl wrote:
>>> Eric,
>>>
>>> On Thu, Feb 13, 2014 at 11:46 AM, Eric McCorkle
>>> <eric.mccorkle at oracle.com> wrote:
>>>> I'll open one.
>>>>
>>>> Sent from my iPhone
>>>
>>> What is the status of this?
>>> The fix is already in the type-annotations repository.
>>>
>>> cu, WMD.
>>>
>>>
>>>>> On Feb 13, 2014, at 11:43 AM, Werner Dietl <wdietl at gmail.com> wrote:
>>>>>
>>>>> Any thoughts on this?
>>>>> Should I open an issue?
>>>>>
>>>>> cu, WMD.
>>>>>
>>>>>> On Fri, Feb 7, 2014 at 7:30 PM, Werner Dietl <wdietl at gmail.com> wrote:
>>>>>> This changeset:
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk9/dev/langtools/rev/d607ae60772d
>>>>>>
>>>>>> contains a re-ordering of how the bytecode is read.
>>>>>>
>>>>>> --- a/src/share/classes/com/sun/tools/javac/jvm/ClassReader.java Sun
>>>>>> Feb 02 12:12:01 2014 +0100
>>>>>> +++ b/src/share/classes/com/sun/tools/javac/jvm/ClassReader.java Mon
>>>>>> Feb 03 17:19:15 2014 -0500
>>>>>>
>>>>>>         // local variable
>>>>>> -        case LOCAL_VARIABLE:
>>>>>> -        // resource variable
>>>>>> -        case RESOURCE_VARIABLE:
>>>>>> -            int table_length = nextChar();
>>>>>> +        case LOCAL_VARIABLE: {
>>>>>> +            final int table_length = nextChar();
>>>>>> +            final TypeAnnotationPosition position =
>>>>>> +                TypeAnnotationPosition.localVariable(readTypePath());
>>>>>> +
>>>>>>             position.lvarOffset = new int[table_length];
>>>>>>             position.lvarLength = new int[table_length];
>>>>>>             position.lvarIndex = new int[table_length];
>>>>>> @@ -1498,67 +1518,142 @@ public class ClassReader {
>>>>>>                 position.lvarLength[i] = nextChar();
>>>>>>                 position.lvarIndex[i] = nextChar();
>>>>>>             }
>>>>>> -            break;
>>>>>> +            return position;
>>>>>> +        }
>>>>>>
>>>>>> The code now reads the TypePath before the local variable information.
>>>>>> This breaks the order in which the ClassWriter writes this information.
>>>>>>
>>>>>> This results in a ClassCastException in unrelated code, because the
>>>>>> ClassReader gets confused about the class file.
>>>>>>
>>>>>> In type-annotations, I changed the code back:
>>>>>>
>>>>>> http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/7e9c983565ac
>>>>>>
>>>>>> and this resolves these exceptions.
>>>>>>
>>>>>> Is there a reason for this change? I think the previous implementation
>>>>>> did the right ordering according to the spec.
>>>>>>
>>>>>> cu, WMD.
>>>>>>
>>>>>> --
>>>>>> http://www.google.com/profiles/wdietl
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> http://www.google.com/profiles/wdietl
>>>
>>>
>>>
> 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 303 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/type-annotations-dev/attachments/20140225/0e0d115f/eric_mccorkle-0001.vcf 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
Url : http://mail.openjdk.java.net/pipermail/type-annotations-dev/attachments/20140225/0e0d115f/signature-0001.asc