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

[harold seigel]

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.

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().

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"

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