RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

Fri Feb 17 16:17:43 UTC 2017

Hi Karen,

Thanks for the review!  I'll fix the comment in AccModuleTest before 
pushing the change.

Harold

On 2/17/2017 11:11 AM, Karen Kinnear wrote:
> Code looks good.
>
> In AccModuleTest, could you fix comment line 32: ClassFormatError -> NoClassDefFoundError
> Thank you for testing inner classes as well.
>
> Agree with Harold et al that this particular check should be unconditional. The meaning of this flag today could have been
> expressed as a different magic number, so if this is a module-info.class file, we don’t want to look any further.
>
> Part of the history of the _need_verify was for javaws which cached pre-verified files. It also is used for trusted class loaders.
> Agreed that it is confusing, and if there is no performance benefit, totally worth removing the format checks on future
> class file versions (so we don’t break code already in the field).
>
> thanks,
> Karen
>
>
>
>> On Feb 17, 2017, at 10:37 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> I agree.  We need a bug to figure out why there's this _need_verify flag in classFileParser.cpp.   I think it was an optimization for classes on the bootclasspath (formerly rt.jar) because they should never have any errors.  But classFileParser does format checks which I can't see why they'd be costly for performance (should be measured as part of the upcoming bug).   The main effect of this _needs_verify flag here is to confuse people looking at the code.
>>
>> I've reviewed this and it looks like a good fix.  To me it makes no difference if checking for module in verify_class_modifiers() is in _needs_verify or not.
>>
>> Thank you for adding comments in the .cod files in the tests.
>>
>> Thanks,
>> Coleen
>>
>> On 2/17/17 9:20 AM, harold seigel wrote:
>>> To clarify about format checks:
>>>
>>> Conditional checks remain conditional because of backward compatibility concerns.  New checks should be unconditional unless there is an explicit reason for them to be conditional.  With ACC_Module, there is no reason for that check to be unconditional.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/17/2017 7:58 AM, harold seigel wrote:
>>>> Please review this new version of the fix for JDK-8174725:
>>>>
>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>>>
>>>> This fix moves the check for ACC_Module to verify_legal_class_modifiers() as required by David.
>>>>
>>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>> That is outside of the scope of this bug.  Feel free to open a new bug for this issue.  Until then, just use common sense or ask Alex Buckley what to do when adding a format check.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>>>> special case to check for ACC_MODULE.
>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>>>
>>>>>> Also, since a classfile version check is already needed in
>>>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>>>> convenient to just do the check and possible throw right there.
>>>>>> However, if you think it's important, I'll move the check to
>>>>>> verify_legal_class_modifiers().
>>>>> It seems like a good place for it (not withstanding it currently returns immediately) and avoid the need to duplicate the code. It is the duplication I disliked - but "verify_legal_class_modifiers" certainly sounds like the place that would check for ACC_MODULE.
>>>>>
>>>>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>>>>> to be a module', how about:
>>>>>>
>>>>>>     "% is not a class because access_flag ACC_MODULE is set"
>>>>> Okay I guess.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>>>
>>>>>>> Also with regards to the error message ... I think it is expressed
>>>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>>>> inability to find the class in the given class representation - I
>>>>>>> suggest:
>>>>>>>
>>>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>>>> ACC_MODULE set.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>>>
>>>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>>>>>> JVM-9 Spec.
>>>>>>>>>>
>>>>>>>>>> Open Web:
>>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>>>
>>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>>>>>> NSK tests.
>>>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>>>> have a class name starting with a lower case letter.
>>>>>>>>>
>>>>>>>>> -Alan