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

[serguei.spitsyn at oracle.com]

On 4/11/13 7:35 PM, David Holmes wrote:
> 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).

It looks Ok for now (JDK 8).
But it can be reconsidered for JDK 9 if the performance impact for 
internal lambda expressions is noticeable.
Anyway, it is good to know about such a possible impact.

Thanks,
Serguei

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