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

[serguei.spitsyn at oracle.com]

Convinced. :)

Thanks,
Serguei

On 4/11/13 11:45 AM, harold seigel wrote:
> Hi Sergui,
>
> Thanks for reviewing this change.
>
> I set a breakpoint at /Verifier::should_verify_for()/, conditioned on 
> /is_anon_loader/ being TRUE, and then ran the Queens program.  The 
> breakpoint did not trigger.  So I don't think any anonymous classes 
> are loaded unless explicitly requested by user code.
>
>     bool Verifier::should_verify_for(oop class_loader, bool
>     should_verify_class, bool is_anon_loader) {
>        return ((class_loader == NULL && !is_anon_loader) ||
>     !should_verify_class) ?
>          BytecodeVerificationLocal : BytecodeVerificationRemote;
>      }
>
> Thanks, Harold
>
> On 4/11/2013 12:40 PM, serguei.spitsyn at oracle.com wrote:
>> On 4/11/13 9:31 AM, serguei.spitsyn at oracle.com wrote:
>>> The fix looks good modulo the bootstrap anonymous classes.
>>
>> To be clear, I've reviewed the second webrev.
>>
>> Thanks,
>> Serguei
>>
>>> How many anonymous classes are loaded by bootstrap CL?
>>> Is it worth to care about?
>>>
>>> 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
>>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130411/b6e3aa4b/attachment.html