Request for review: JDK-8011260: fatal error: LineNumberTable attribute has wrong length in class file

[David Holmes]

On 12/04/2013 12:00 PM, David Holmes wrote:
> On 12/04/2013 9:47 AM, serguei.spitsyn at oracle.com wrote:
>> On 4/11/13 4:08 PM, David Holmes wrote:
>>> On 12/04/2013 2:31 AM, serguei.spitsyn at oracle.com wrote:
>>>> The fix looks good modulo the bootstrap anonymous classes.
>>>> How many anonymous classes are loaded by bootstrap CL?
>>>> Is it worth to care about?
>>>
>>> Serguei: Is what worth caring about?
>>>
>>> As has been explained to me we will now verify all anonymous classes
>>> (bootstrap or not). Is it performance you are concerned about?
>>
>> Yes, performance.
>
> Okay. As this would seem to impact all lambda's I also have to be
> concerned about the performance impact.

Correction this only affects lambdas associated with boot-loader types, 
so less of a concern (at least for JDK 8, but JDK 9 we will likely have 
more internal use of lambda).

David

> David
> -----
>
>> I also wanted to understand what classes are potentially impacted.
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> Harold: based on current understanding this looks good to me.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 4/11/13 5:51 AM, harold seigel wrote:
>>>>> Hi David,
>>>>>
>>>>> >> /But if I understand the fix correctly we are now verifying all
>>>>> anonymous classes regardless of which loader they are associated - is
>>>>> that right? /
>>>>>
>>>>> Yes, that is right.  We will now verify all anonymous classes
>>>>> regardless of loader.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 4/10/2013 10:42 PM, David Holmes wrote:
>>>>>> On 11/04/2013 12:27 AM, harold seigel wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> Thanks for looking at this.  To clarify, by "anonymous classes" I
>>>>>>> mean
>>>>>>> only classes loaded by the anonymous class loader, not anonymous
>>>>>>> inner
>>>>>>> classes.
>>>>>>
>>>>>> Of course :)
>>>>>>
>>>>>>> Please see in-lined responses.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>> On 4/9/2013 8:03 PM, David Holmes wrote:
>>>>>>>> Hi Harold,
>>>>>>>>
>>>>>>>> On 9/04/2013 11:55 PM, harold seigel wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review the following bug fix:
>>>>>>>>>
>>>>>>>>> Summary:   The assertion was prevented by changing the code to
>>>>>>>>> verify
>>>>>>>>> classes loaded by the anonymous class loader, unless -noverify was
>>>>>>>>
>>>>>>>> So if I understand this right:
>>>>>>>>
>>>>>>>> - previously: loader == NULL -> BytecodeVerificationLocal
>>>>>>>> - now: loader == NULL && anonymous-loader ->
>>>>>>>> BytecodeVerificationRemote
>>>>>>>>
>>>>>>>> in other words we were skipping verification for anonymous classes.
>>>>>>> Yes, unless full verification (-verify) was explicitly specified or
>>>>>>> their host classes had their own non-null class loaders.
>>>>>>
>>>>>> I'm still a little confused - sorry. Normally we don't verify
>>>>>> boot-loader classes (unless requested). So I would have expected that
>>>>>> anonymous classes associated with the boot-loader would also not get
>>>>>> verified. But if I understand the fix correctly we are now verifying
>>>>>> all anonymous classes regardless of which loader they are associated
>>>>>> - is that right?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>>
>>>>>>>> I guess I don't fully understand the implications here. Given we
>>>>>>>> normally skip verification for classes loaded by the boot-loader,
>>>>>>>> does
>>>>>>>> this mean we now verify anonymous classes "linked" to the
>>>>>>>> boot-loader?
>>>>>>>> Are all anonymous classes considered to be non-boot-loader classes?
>>>>>>>>
>>>>>>> Yes, we would now verify anonymous classes "linked" to the
>>>>>>> boot-loader.
>>>>>>>
>>>>>>> The answer to 'should anonymous calls considered to be
>>>>>>> non-boot-loader
>>>>>>> classes" might be yes and no, depending on the context. And this
>>>>>>> change
>>>>>>> proposes that the answer be 'no' when it comes to whether or not to
>>>>>>> skip
>>>>>>> verification.
>>>>>>>
>>>>>>>> Makes me wonder if there are other places where we check a loader
>>>>>>>> for
>>>>>>>> NULL but also need to check it is not related to anonymous
>>>>>>>> classes ?
>>>>>>>>
>>>>>>> That's a good point.  Should I create a new bug to look into this?
>>>>>>>>> specified.  Also, the relevant assertion messages were improved to
>>>>>>>>> include additional information, including class name.
>>>>>>>>
>>>>>>>> Thanks for that. I hadn't realized the classfile_parse_error
>>>>>>>> routines
>>>>>>>> created and threw exceptions.
>>>>>>>>
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8011260/
>>>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8011260/>
>>>>>>>>>
>>>>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8011260
>>>>>>>>>
>>>>>>>>> The change was tested by hand to ensure that the assertion no
>>>>>>>>> longer
>>>>>>>>> occurs when the test is run.  Additional testing was done using
>>>>>>>>> JCK Lang
>>>>>>>>> and VM, JTREG tests, ute vm.quick.testlist and vm.mlvm.testlist
>>>>>>>>> tests,
>>>>>>>>> and JPRT tests.
>>>>>>>>>
>>>>>>>>> Thank you!
>>>>>>>>> Harold
>>>>>>>
>>>>>
>>>>
>>